条件太多的清洁代码

Question

我从前段时间发现了这段丑陋的代码:

@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");
    }
}

我现在知道它有几个主要缺陷:

• 方法做得太多而且时间过长,
• 它有太多的条件语句,所以很容易迷路,
• 不必要的评论使代码不易读
• 重复的代码,
• 可能混合的复杂程度.
• 测试它会很可怕.
• 别的什么？

我怎样才能重构它以使其干净简单？

Answer 1

我对另一个答案略有不同.我将验证与方法中的其他逻辑分开.这意味着您不必阅读大量的if语句来查看方法的核心逻辑,如果要更改验证,则只需在一个位置更新一个语句.例如,对于第一部分,添加一个私有方法:

private PizzaSizeData getAndVerifySelectedPizza() {
    if (comboPizza.getValue() == null) {
        infoTip.showTip(comboPizza, "select pizza");
        return null;
    }

    if (comboPizzaSize.getValue() == null) {
        infoTip.showTip(comboPizzaSize, "select pizza size");
        return null;
    }

    PizzaData selectedPizza = getPizzaData(comboPizza.getValue());
    if (selectedPizza == null) {
        ExceptionDialog exceptionDialog = new ExceptionDialog("Error when searching for selected pizza", new Exception());
        exceptionDialog.showAndWait();
        return null;
    }

    return getPizzaSizeData(comboPizzaSize.getValue());
}

您可以返回选项而不是null,但这说明了该机制.

然后调用新方法:

private void buttSellAction(ActionEvent event){
    InfoTip infoTip = new InfoTip();

    PizzaSizeData selectedPizzaSize = getAndVerifySelectedPizza();
    if (selectedPizzaSize == null) {
        return;
    }

    // Carry on with the method....

在具有早期return语句的方法的开头进行验证是一种常见模式,因此多次返回不会混淆任何人,并且它们允许每个验证规则单独编写.