Eliminate If-Else with Strategy Pattern | A Summary of the Smart-Auto Refactoring

By Feng Wang (Weifeng)

1. Background

The smart-auto tool has undergone significant development over the years, thanks to the efforts of multiple testers. It now boasts robust features, including the ability to compare return values from various HSF interface calls and assert. In fact, it executes over 55% of Taobao Maicai's automated test cases daily. However, as new features were added, the codebase grew increasingly complex, making maintenance more challenging. This article focuses on refactoring the core code related to smart-auto's interface testing to make it more readable and maintainable.

2. Analysis of the Current Situation

Let's take a look at the core code related to interface testing before optimization. In simple terms, the code can be broken down as follows:

(1) HsfCheck1, HsfCheck2, HsfCheck3, and HsfCheck4 are used for HSF interface check.

(2) All interface check methods are contained within a single handler, with different methods distinguished by complex if-else branches.

(3) The overall code is about 200 lines.

public CheckOutputModel doHandle(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel) throws Exception {
     if(!jsonPathList.isEmpty()){
          if (attributeModel.isCompare()) {
                HsfCheck1
          }
     }else{
         if(attributeModel.isCompare()) {
                HsfCheck2
         }
     }
     if ( checkConfigStatusObject == null ||checkConfigStatusObject.isEmpty() || checkConfigStatusObject.getBoolean("isCheck") == false  ){
                return result;
            }else{
             if(assertExpectStr == null || !node.isCustom()){
                HsfCheck3
             }else{
                HsfCheck4
             }
            }
 }

The complete code is as follows:

@Service("commonCheckHandlerAbandon")
public class CommonCheckHandler implements CheckHandler{
    @Resource
  private CaseConfigHandlerService caseConfigHandlerService;
    @Resource
    private CheckDataCaseService checkDataCaseService;
    private Logger logger = LoggerFactory.getLogger(this.getClass());
    @Override
    public CheckOutputModel doHandle(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel) throws Exception {
        ThubNodeConfig node = JSON.parseObject(checkRecordModel.getTestsuiteDO().getStepConfig(), ThubNodeConfig.class);
        TestsuiteAttributeModel attributeModel = JSON.parseObject(checkRecordModel.getAttributes(), TestsuiteAttributeModel.class);
        if (checkRecordModel.getTestsuiteDO().getShadow()) {
            // Pressure indicators of the full trace
            EagleEye.putUserData("t", "1");
        }
        CheckOutputModel result = new CheckOutputModel();
        if(node==null){
            result.setSuccess(false);
            return result;
        }
        List<String> jsonPathList = Collections.emptyList();
        if(node.getJsonPath() != null && !node.getJsonPath().trim().isEmpty() ){
            jsonPathList = Arrays.asList(node.getJsonPath().split(";"));
        }
        try{
            //If the jsonPathList parameter is not empty, execute the jsonPath for parsing and perform the comparison after the execution of jsonPath.
            if(!jsonPathList.isEmpty()){
                List<CheckDiffModel> totalFailInfo = new ArrayList<>();
                List<String> errorMessages = new ArrayList<>(); // It is used to store error messages
                for (String jsonPath  : jsonPathList) {
                    try {
                        if (attributeModel.isCompare()) {
                            String actualResultStr = StringEscapeUtils.unescapeJavaScript((String) executeResultModel.getValueByKey(CheckCaseInfoConst.ACTUAL_RESULT));
                            String expectResultStr = StringEscapeUtils.unescapeJavaScript((String) executeResultModel.getValueByKey(CheckCaseInfoConst.EXPECT_RESULT));
                            Object actualResult = null;
                            Object expectResult = null;
                            if (StringUtils.isNoneBlank(actualResultStr)) {
                                Object actualValueObject = JsonPath.read(actualResultStr, jsonPath);
                                String actualValue = JSON.toJSONString(actualValueObject);
                                if (JSON.isValidObject(actualValue)) {
                                    actualResult = JSON.parseObject(actualValue);
                                } else if (JSON.isValidArray(actualValue)) {
                                    actualResult = JSON.parseArray(actualValue);
                                } else {
                                    actualResult = JSON.parse(actualValue);
                                }
                            }
                            if (StringUtils.isNoneBlank(expectResultStr)) {
                                Object expectValueObject = JsonPath.read(expectResultStr, jsonPath);
                                String expectValue = JSON.toJSONString(expectValueObject);
                                if (JSON.isValidObject(expectValue)) {
                                    expectResult = JSON.parseObject(expectValue);
                                } else if (JSON.isValidArray(expectValue)) {
                                    expectResult = JSON.parseArray(expectValue);
                                } else {
                                    expectResult = JSON.parse(expectValue);
                                }
                            }
                            StringBuffer ignorBuffer = new StringBuffer();
                            ignorBuffer.append(node.getIgnorConfig());
                            List<CheckDiffModel> failInfo = QAssert.getReflectionDiffInfo("assert diff", expectResult, actualResult, ignorBuffer.toString(),
                                    ReflectionComparatorMode.LENIENT_ORDER, ReflectionComparatorMode.LENIENT_DATES, ReflectionComparatorMode.IGNORE_DEFAULTS);
                            failInfo.forEach(i -> i.setNodeName(jsonPath + "---" + i.getNodeName()));
                            totalFailInfo.addAll(failInfo);
                        }
                    } catch (Exception e) {
                        // Record error messages
                        String errorMessage = "Error with JSON path: " + jsonPath + " - " + e.getMessage();
                        errorMessages.add(errorMessage);
                        logger.error(errorMessage, e);
                    }
                }
                if (!totalFailInfo.isEmpty()||!errorMessages.isEmpty()) {
                    if(!totalFailInfo.isEmpty()){
                        errorMessages.add(0, "value not same");
                    }
                    // Combine error messages, and separate them by carriage returns
                    String combinedErrorMessages = String.join("\n", errorMessages);
                    result.setSuccess(false);
                    result.setErrorCode(combinedErrorMessages);
                    result.setFailInfoList(totalFailInfo);
                } else {
                    result.setSuccess(true);
                }
// If jsonPathList is empty, follow the normal comparison logic
            }else {
                result.setTraceId(EagleEye.getTraceId());
                if(attributeModel.isCompare()) {
                    String actualResultStr = StringEscapeUtils.unescapeJavaScript((String) executeResultModel.getValueByKey(CheckCaseInfoConst.ACTUAL_RESULT));
                    String expectResultStr = StringEscapeUtils.unescapeJavaScript((String) executeResultModel.getValueByKey(CheckCaseInfoConst.EXPECT_RESULT));
                    Object actualResult = null;
                    Object expectResult = null;
                    if (StringUtils.isNoneBlank(actualResultStr)) {
                        if (JSON.isValidObject(actualResultStr)) {
                            actualResult = JSON.parseObject(actualResultStr);
                        } else if (JSON.isValidArray(actualResultStr)) {
                            actualResult = JSON.parseArray(actualResultStr);
                        } else {
                            actualResult = JSON.parse(actualResultStr);
                        }
                    }
                    if (StringUtils.isNoneBlank(expectResultStr)) {
                        if (JSON.isValidObject(expectResultStr)) {
                            expectResult = JSON.parseObject(expectResultStr);
                        } else if (JSON.isValidArray(expectResultStr)) {
                            expectResult = JSON.parseArray(expectResultStr);
                        } else {
                            expectResult = JSON.parse(expectResultStr);
                        }
                    }
                    StringBuffer ignorBuffer = new StringBuffer();
                    ignorBuffer.append(node.getIgnorConfig());
                    List<CheckDiffModel> failInfo = QAssert.getReflectionDiffInfo("assert diff", expectResult, actualResult, ignorBuffer.toString(),
                            ReflectionComparatorMode.LENIENT_ORDER, ReflectionComparatorMode.LENIENT_DATES, ReflectionComparatorMode.IGNORE_DEFAULTS);
                    if (!failInfo.isEmpty()) {
                        result.setSuccess(false);
                        result.setErrorCode("value not same");
                        result.setFailInfoList(failInfo);
                    } else {
                        result.setSuccess(true);
                    }
                }
            }
            // Perform assertion check
            JSONObject checkConfigStatusObject = JSON.parseObject(checkRecordModel.getTestsuiteDO().getCheckConfigStatus());
// Return directly without assertion
            if ( checkConfigStatusObject == null ||checkConfigStatusObject.isEmpty() || checkConfigStatusObject.getBoolean("isCheck") == false  ){
                return result;
            }else{
// Perform assertion check
                String assertActualStr = StringEscapeUtils.unescapeJavaScript((String)executeResultModel.getValueByKey(CheckCaseInfoConst.ACTUAL_RESULT));
                String assertExpectStr = StringEscapeUtils.unescapeJavaScript((String)executeResultModel.getValueByKey(CheckCaseInfoConst.EXPECT_RESULT));
                CheckDataCaseDO checkDataCaseDO = caseParam.getCaseDO();
                // Assertion comparison
                if(assertExpectStr == null || !node.isCustom()){
                    boolean checkResult = caseConfigHandlerService.resultAssert(checkRecordModel.getTestsuiteDO(), checkDataCaseDO,assertActualStr);
                    if (!checkResult){
                        result.setSuccess(false);
                        return result;
                    }
                    CheckDataCaseQueryDO checkDataCaseQueryDO = new CheckDataCaseQueryDO();
                    checkDataCaseQueryDO.setId(checkDataCaseDO.getId());
                    List<CheckCaseResult> checkResultList = checkDataCaseService.queryCheckCaseResult(checkDataCaseQueryDO).getResult();
                    List<CheckDiffModel> checkCoonfigFailInfo = new ArrayList<>();
                    for (CheckCaseResult checkCaseResult : checkResultList) {
                        String checkConfigResult = checkCaseResult.getCheckConfigResult();
                        List<Map> checkParse = JSONArray.parseArray(checkConfigResult, Map.class);
                        for (Map map : checkParse) {
                            CheckDiffModel checkDiffModel = new CheckDiffModel();
                            String checkConfig = String.valueOf(map.get("checkConfigResult"));
                            StringBuffer stringBuffer = new StringBuffer();
                            if(!StringUtils.equals(checkConfig,"true")){
                                stringBuffer.append((String)map.get("assertNode")+map.get("assertCondition")+map.get("assertErpect"));
                                checkDiffModel.setActualValue("false");
                                checkDiffModel.setNodeName(String.valueOf(stringBuffer));
                                checkCoonfigFailInfo.add(checkDiffModel);
                            }
                        }
                    }
                    if (checkCoonfigFailInfo.size() != 0) {
                        result.setSuccess(false);
                        result.setErrorCode("value not same");
                        result.setFailInfoList(checkCoonfigFailInfo);
                    } else{
                        result.setSuccess(true);
                    }
                    // Cross-application assertion check
                }else {
                    boolean checkResult = caseConfigHandlerService.resultAssertComp(checkRecordModel.getTestsuiteDO(), checkDataCaseDO, assertActualStr,assertExpectStr);
                    if (!checkResult){
                        result.setSuccess(false);
                        return result;
                    }
                    CheckDataCaseQueryDO checkDataCaseQueryDO = new CheckDataCaseQueryDO();
                    checkDataCaseQueryDO.setId(checkDataCaseDO.getId());
                    List<CheckCaseResult> checkResultList = checkDataCaseService.queryCheckCaseResult(checkDataCaseQueryDO).getResult();
                    List<CheckDiffModel> checkCoonfigFailInfo = new ArrayList<>();
                    for (CheckCaseResult checkCaseResult : checkResultList) {
                        String checkConfigResult = checkCaseResult.getCheckConfigResult();
                        List<Map> checkParse = JSONArray.parseArray(checkConfigResult, Map.class);
                        CheckDiffModel checkDiffModel = new CheckDiffModel();
                        StringBuffer stringBuffer = new StringBuffer();
                        for (Map map : checkParse) {
                            Boolean checkConfig = (Boolean) map.get("checkConfigResult");
                            if(!checkConfig){
                                stringBuffer.append((String)map.get("assertNode")+map.get("assertCondition")+map.get("assertErpect"));
                                stringBuffer.append(",");
                                checkDiffModel.setActualValue("false");
                            }
                        }
                        checkDiffModel.setNodeName(String.valueOf(stringBuffer));
                        checkCoonfigFailInfo.add(checkDiffModel);
                    }
                    if (checkCoonfigFailInfo.get(0).getActualValue() != null) {
                        result.setSuccess(false);
                        result.setErrorCode("value not same");
                        result.setFailInfoList(checkCoonfigFailInfo);
                    } else{
                        result.setSuccess(true);
                    }
                }
                }
        }catch(Exception e){
            e.printStackTrace();
            result.setSuccess(false);
            result.setMsgInfo(e.getMessage());
        }finally{
            EagleEye.removeUserData("t");
        }
        return result;
    }
}

The above code has several issues:

(1) It's composed of lengthy if-else branch judgments, making the logic confusing and coupling all four HSF interface checks together without extensibility. Any subsequent addition of functions requires modifying the original coupled code, which may affect the original function.

(2) This code does not follow the open/closed principle (OCP). A good code needs to be open to extension and closed to modification.

(3) All the logic for implementing the HSF check is in the same handler class. As a result, there is a lot of code in this class, which affects the readability and maintainability of the code.

(4) The if-else condition of this code is difficult to understand, and it is impossible to determine which HSF check type is checked in a certain condition. It always takes a long time to review this code.

3. Solution

To address these issues, we can use the strategy factory pattern, which involves encapsulating each HSF check method and then routing and issuing it through the strategy factory pattern. This pattern decouples the lengthy code and establishes a framework, ensuring the extensibility of the code. Let's take a look at the code.

First, we'll create a strategy factory class:

public class CheckStrategyFactory {
    private final Map<CheckStrategySelector, HsfInterfaceCheck> strategyRegistry = new HashMap<>();

    @Autowired
    public CheckStrategyFactory(HsfAssertCheck hsfAssertCheck,
                                HsfCrossInterfaceAssertCompare hsfCrossInterfaceAssertCompare,
                                HsfFullCompareCheck hsfFullCompareCheck,
                                HsfMultipleJsonPathCompareCheck hsfMultipleJsonPathCompareCheck,
                                JsonPathCompareStrategySelector jsonPathCompareStrategySelector,
                                CrossInterfaceAssertCompareStrategySelector crossInterfaceAssertCompareStrategySelector,
                                FullCompareStrategySelector fullCompareStrategySelector,
                                AssertStrategySelector assertStrategySelector) {

        // Register all strategies in the construction function or initialization block
        strategyRegistry.put(assertStrategySelector, hsfAssertCheck);
        strategyRegistry.put(crossInterfaceAssertCompareStrategySelector, hsfCrossInterfaceAssertCompare);
        strategyRegistry.put(fullCompareStrategySelector, hsfFullCompareCheck);
        strategyRegistry.put(jsonPathCompareStrategySelector, hsfMultipleJsonPathCompareCheck);
        // ... Register more strategies ...
    }
    public HsfInterfaceCheck getStrategy(ThubNodeConfig node, JSONObject checkConfigStatusObject,TestsuiteAttributeModel attributeModel , ExecuteResultModel executeResultModel) {

        for (Map.Entry<CheckStrategySelector, HsfInterfaceCheck> entry : strategyRegistry.entrySet()) {
            if (entry.getKey().matches(node, checkConfigStatusObject, attributeModel, executeResultModel)) {
                return entry.getValue();
            }
        }

        return null; // A fallback check strategy by returning null
    }
}

Create two interfaces: one is the CheckStrategySelector for strategy selection, and the other is the HsfInterfaceCheck for HSF check.

public interface CheckStrategySelector {
    boolean matches(ThubNodeConfig node, JSONObject checkConfigStatusObject , TestsuiteAttributeModel attributeModel , ExecuteResultModel executeResultModel);
}

public interface HsfInterfaceCheck {
    CheckOutputModel  check(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel);
}

Create four strategy classes and four HSF check classes to respectively implement the strategy selection interface CheckStrategySelector and the HSF check interface HsfInterfaceCheck.

The following are the HsfCheck1 and HsfCheck2 strategy selection classes, with the remaining two classes omitted for brevity.

public class AssertStrategySelector implements CheckStrategySelector {
    @Override
    public boolean matches(ThubNodeConfig node, JSONObject checkConfigStatusObject, TestsuiteAttributeModel attributeModel , ExecuteResultModel executeResultModel) {
        String assertExpectStr = StringEscapeUtils.unescapeJavaScript((String)executeResultModel.getValueByKey(CheckCaseInfoConst.EXPECT_RESULT));
        return !(checkConfigStatusObject == null ||checkConfigStatusObject.isEmpty() || checkConfigStatusObject.getBoolean("isCheck") == false) && (assertExpectStr == null || !node.isCustom());
    }
}

public class FullCompareStrategySelector implements CheckStrategySelector {
    @Override
    public boolean matches(ThubNodeConfig node, JSONObject checkConfigStatusObject, TestsuiteAttributeModel attributeModel , ExecuteResultModel executeResultModel) {
        return attributeModel.isCompare() && (node.getJsonPath() == null || node.getJsonPath().trim().isEmpty());
    }
}

The following are the HsfCheck1 and HsfCheck2 check classes, with the remaining two classes omitted for brevity.

@Service("hsfAssertCheck")
public class HsfAssertCheck implements HsfInterfaceCheck {
    @Resource
    private CaseConfigHandlerService caseConfigHandlerService;
    @Resource
    private CheckDataCaseService checkDataCaseService;
    @Override
    public CheckOutputModel check(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel) {

}

@Service("hsfFullCompareCheck")
public class HsfFullCompareCheck implements HsfInterfaceCheck {

    @Override
    public CheckOutputModel check(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel) {

    }
}

Finally, the Handler code is simplified to this.

@Service("commonCheckHandler")
public class CommonCheckHandler implements CheckHandler{
    private final CheckStrategyFactory factory;

    public CommonCheckHandler(CheckStrategyFactory factory) {
        this.factory = factory;
    }


    @Override
    public CheckOutputModel doHandle(CheckCaseModel caseParam, BuildTestsuiteModel checkRecordModel, ExecuteResultModel executeResultModel) throws Exception {
        ThubNodeConfig node = JSON.parseObject(checkRecordModel.getTestsuiteDO().getStepConfig(), ThubNodeConfig.class);
        TestsuiteAttributeModel attributeModel = JSON.parseObject(checkRecordModel.getAttributes(), TestsuiteAttributeModel.class);
        JSONObject checkConfigStatusObject = JSON.parseObject(checkRecordModel.getTestsuiteDO().getCheckConfigStatus());
        CheckOutputModel result = new CheckOutputModel();
        if(node==null){
            result.setSuccess(false);
            return result;
        }

        HsfInterfaceCheck hsfInterfaceCheckStrategy = factory.getStrategy(node, checkConfigStatusObject, attributeModel, executeResultModel);
        if(hsfInterfaceCheckStrategy != null){
            return hsfInterfaceCheckStrategy.check(caseParam, checkRecordModel, executeResultModel);
        } else {
            result.setSuccess(false);
            result.setErrorCode("The corresponding check strategy is not found.");
            return result;
        }
    }
}

The above code is refactored into multiple files, and the lengthy if-else code is broken down using the strategy factory pattern. Let's examine whether the refactored code is an improvement. The following figure shows the overall logic of the refactoring:

After refactoring, a factory class is created, which determines the strategy type through its strategy judgment logic. It dynamically decides which strategy to use at runtime and routes to the corresponding check method.

(1) The refactored code adheres to the open/closed principle, minimizing code changes when adding new strategies and reducing the risk of introducing bugs.

(2) The code complexity is significantly reduced by decoupling the definition, creation, and usage of strategies. This approach keeps each section of code concise and easy to manage.

(3) The refactored code greatly enhances code readability and maintainability.

4. Summary

Of course, not all if-else branches are indicative of poor coding. If the if-else branches are straightforward and the code isn't excessive, there’s no issue. Adhering to the KISS principle (Keep It Simple, Stupid) is often the best approach. However, when you're faced with numerous if-else branches, each with its complex logic, it may be time to consider whether the strategy pattern could help streamline and clarify your code, enhancing its maintainability.

Disclaimer: The views expressed herein are for reference only and don't necessarily represent the official views of Alibaba Cloud.

Community

Eliminate If-Else with Strategy Pattern | A Summary of the Smart-Auto Refactoring

1. Background

2. Analysis of the Current Situation

3. Solution

4. Summary

TheRock

You may also like

Comments

TheRock

Related Products

Web Hosting Solution

YiDA Low-code Development Platform

mPaaS

Web Hosting