samedi 20 août 2016

Cleaning code with too much conditionals

I've found this ugly piece of code from some time ago:

@FXML
private void buttSellAction(ActionEvent event){
    InfoTip infoTip = new InfoTip();
    if(comboPizza.getValue() != null){
        if(comboPizzaSize.getValue() != null){
            PizzaData selectedPizza = getPizzaData(comboPizza.getValue());
            PizzaSizeData selectedPizzaSize = getPizzaSizeData(comboPizzaSize.getValue());
            Date date = new Date();
            Timestamp timestamp = new Timestamp(date.getTime());
            if( selectedPizza != null ){                    
                if(groupDelivery.getSelectedToggle().equals(radioNo)){ // sale without delivery
                    Alert alert = new Alert(AlertType.CONFIRMATION);
                    alert.setTitle("Confirm");
                    alert.setHeaderText("Total cost: " + String.format("%.2f", selectedPizza.getPrice() + selectedPizzaSize.getPrice()));
                    alert.setContentText("Proceed with sale?");

                    Optional<ButtonType> result = alert.showAndWait();
                    if (result.get() == ButtonType.OK){
                        insertSale(timestamp, currentUser.getLogin(), selectedPizza.getID(), 
                                   selectedPizzaSize.getSize(), false, selectedPizza.getPrice() + selectedPizzaSize.getPrice());

                        infoTip.getInfoTip().setId("greenInfoTip");
                        infoTip.showTip((Button)event.getSource(), "   Saved   ");
                    }
                }else{ //Sale with delivery
                    String adress = textFAdress.getText();
                    String clientName = textFClientName.getText();
                    String telephone = textFTelephone.getText();
                    String deliveryCost = textFCost.getText();

                    boolean isAdressOK = ((adress.length() < 51) && (adress.isEmpty() == false))? true: false;
                    boolean isClientNameOK = (clientName.length() < 36)? true: false;
                    boolean isTelephoneOK = ((telephone.length() < 21) && (telephone.isEmpty() == false))? true: false;
                    boolean isCostOK;
                    try{ Double.valueOf(deliveryCost); isCostOK = true; }
                    catch(NumberFormatException exception){ isCostOK = false; }

                    if(isAdressOK == true){
                        if(isClientNameOK == true){
                            if(isTelephoneOK == true){
                                if(isCostOK == true){
                                    double totalCost = selectedPizza.getPrice() + selectedPizzaSize.getPrice() + Double.valueOf(deliveryCost);
                                    //everything is okey
                                    Alert alert = new Alert(AlertType.CONFIRMATION);
                                    alert.setTitle("Confirm");
                                    alert.setHeaderText("Total cost: " + totalCost);
                                    alert.setContentText("Proceed with sale?");

                                    Optional<ButtonType> result = alert.showAndWait();
                                    if (result.get() == ButtonType.OK){
                                        int id = insertSale(timestamp, currentUser.getLogin(), selectedPizza.getID(), 
                                                selectedPizzaSize.getSize(), true, selectedPizza.getPrice() + selectedPizzaSize.getPrice());

                                        insertDelivery(id, adress, clientName, telephone, Double.valueOf(deliveryCost));

                                        infoTip.getInfoTip().setId("greenInfoTip");
                                        infoTip.showTip((Button)event.getSource(), "   Saved   ");
                                    } else {
                                        // ... user chose CANCEL or closed the dialog
                                    }
                                }else{ //cost not ok
                                    infoTip.showTip(textFCost, "keep right format e.g. 4.35");
                                }
                            }else{ //telephone not ok
                                infoTip.showTip(textFTelephone, "max 20 characters, not empty");
                            }
                        }else{ //client name not ok
                            infoTip.showTip(textFClientName, "max 35 characters");
                        }
                    }else{ //adress not ok
                        infoTip.showTip(textFAdress, "max 50 characters, not empty");
                    }
                }
            }else{ //couldnt found selected pizza in pizzaList(which should not be possible)
                ExceptionDialog exceptionDialog = new ExceptionDialog("Error when searching for selected pizza", new Exception());
                exceptionDialog.showAndWait();
            }
        }else{ //pizza size not choosen
            infoTip.showTip(comboPizzaSize, "select pizza size");
        }
    }else{ //pizza not choosen
        infoTip.showTip(comboPizza, "select pizza");
    }
}

I know now it has few major flaws:

  • method is doing too much and its too long,
  • it has too many conditional statements so it is easy to get lost,
  • unnecessary comments making code less readable.
  • repeated code,
  • possibly mixed levels of complexity.
  • something else??

How can I refactor it to make it clean and simple?

Aucun commentaire:

Enregistrer un commentaire