Code Smell - High Cyclomatic Complexity with Multi-Layer Nesting

By Nie Xiaolong (Shuaige)

Introduction

Cyclomatic complexity [1] is the measure of code complexity proposed by Thomas J. McCabe, Sr. in 1976. The more conditional branches, the higher the cyclomatic complexity. As such, the test is more difficult to overwrite and maintain. With the evolution of business and the addition and adjustment of code, if you only add new logic to your original logic, a super-high nested code will grow.

This kind is not a minority in our aged code. The cost of adding a conditional branch is relatively low, which allows you to complete your logic without understanding the original logic. However, it will give some risks to the system. Until one day, we have no idea what the modified line of code affected.

Bad Smell

When performing a business requirement, I ran into this aged code and took one of the values through its high nesting. Since the data structure is complex, the original author wrote a lot of conditional judgments to ensure the availability of the code, forming an ultra-complex code.

/**
 * Parse the [End Reason] in the ticket ACTION data.
 * @param caseId Ticket ID
 * @return
 */
private String queryResolveAction(Long caseId) {
    ActionQueryBizDTO actionQueryBizDTO = new ActionQueryBizDTO();
    actionQueryBizDTO.setBizId(caseId);   //Ticket ID
    actionQueryBizDTO.setDataSource(1);
    ActionQueryDTO actionQueryDTO = new ActionQueryDTO();
    actionQueryDTO.setBizDTOs(Lists.newArrayList(actionQueryBizDTO));
    Result<PageWithData<ActionDTO>> result = ltppActionQueryService.queryActions(actionQueryDTO);
    log.info("query action results:{}", JSON.toJSONString(result));
    if (result.isSuccess() && result.getData() != null) {
        if (CollectionUtils.isNotEmpty(result.getData().getData())) {
            for (ActionDTO actionDTO : result.getData().getData()) {
                if (ACTION_COMPLETE_CODE.equals(actionDTO.getActionCode())) {
                    JSONObject memoObject = JSON.parseObject(actionDTO.getMemo());
                    JSONArray actionKeyMemoArray = memoObject.getJSONArray("actionKeyMemo");
                    for (Object actionKey : actionKeyMemoArray) {
                        Map<String, Object> actionKeyMap = (Map<String, Object>)actionKey;
                        if (MapUtils.isNotEmpty(actionKeyMap) && COMPLETE_REASON.equals(actionKeyMap.get("key"))) {
                            return String.valueOf(actionKeyMap.get("value"));
                        }
                    }
                }
            }
        }
    }
    log.warn("cannot find action given case id {}, the result is {}", caseId, JSON.toJSONString(result));
    return null;
}

Refactoring Ideas

1. Guard Clause Returned to Reduce the Nesting Tier

Guard clause [2] is an optimization scheme to improve nested code, prioritizing the judgment of certain guard conditions to simplify the process of the program.

public static String getCaseQuestionTitle(CaseTaskRelatedDO caseTask){
    Map<String, Object> extAttrs = caseTask.getExtAttrs();
    if(extAttrs == null || extAttrs.isEmpty()){
        return null;
    }
    JSONObject xform = JSON.parseObject(String.valueOf(extAttrs.get("xform")));
    if(xform == null){
        return null;
    }
    JSONObject body = xform.getJSONObject("body");
    if(body == null){
        return null;
    }
    return body.getString("question_title");
}

2. Function Features Converged and Single Responsibility Principle

The single responsibility principle [3] emphasizes that a class should only have one reason for the change and only take one responsibility. It was first proposed by Robert C. Martin in Agile Software Development [4] and became one of the five object-oriented design principles.

/**
 * Query ticket Action information.
 * K,V -> ACTION_CODE,ACTION
 * @param caseId
 * @return
 */
private Map<Integer, ActionDTO> queryCaseActionMap(Long caseId){
    ActionQueryBizDTO actionQueryBizDTO = new ActionQueryBizDTO();
    actionQueryBizDTO.setBizId(caseId);
    ActionQueryDTO actionQueryDTO = new ActionQueryDTO();
    actionQueryDTO.setBizDTOs(Lists.newArrayList(actionQueryBizDTO));
    Result<PageWithData<ActionDTO>> result = ltppActionQueryService.queryActions(actionQueryDTO);
    log.info("query action results:{}", JSON.toJSONString(result));
    if(noActionResult(result)){
        return null;
    }
    List<ActionDTO> actionList = result.getData().getData();
    return actionList.stream().collect(Collectors.toMap(ActionDTO::getActionCode, action -> action));
}

3. Complex Logical Abstraction and Explicit Business Semantics

Programs are meant to be ready by humans and only incidentally for computers to execute.

---Father of Artificial Intelligence: Donald Ervin Knuth

The same functional statement may be the same code after being converted into assembly. However, different forms of expression will bring different understanding costs for the reader.

/**
 * Ticket without ACTION data.
 * @param result
 * @return
 */
private boolean noActionResult(Result<PageWithData<ActionDTO>> result){
    if(result == null){
        return true;
    }
    if(!result.isSuccess()){
        return true;
    }
    if(result.getData() == null){
        return true;
    }
    if(CollectionUtils.isEmpty(result.getData().getData())){
        return true;
    }
    return false;
}

4. Separation of Concerns and Abstract Parser Model

Separation of concerns [5] is a design principle that separates computer programs into different parts for processing. This concept was first proposed by Dijkstra Edsger in his article on the role of scientific thought [6] in 1974. Separating concerns makes the code that solves domain-specific problems separate from the business logic. The smaller the focused problem, the lower the complexity and the easier it is to be solved.

/**
 * Ticket resolution tool class.
 * @author niexiaolong
 * @date 2022/8/24
 */
public class CaseParser {

    /**
     * Parse the conclusion of the "End" status of the ticket.
     * @param actionDTO The status set of the ticket.
     * @return "End" conclusion
     */
    private static String parseCompleteConsequence(ActionDTO actionDTO){
        JSONObject action = JSON.parseObject(actionDTO.getMemo());
        if(action == null){
            return null;
        }
        JSONArray actionKeyArray = action.getJSONArray(ACTION_KEY_MEMO);
        if(actionKeyArray == null || actionKeyArray.isEmpty()){
            return null;
        }
        for (int i=0; i<actionKeyArray.size(); i++){
            JSONObject actionKey = actionKeyArray.getJSONObject(i);
            if(actionKey != null && actionDataKey.equals(actionKey.getString(CaseCodeConstant.COMPLETED_DESC_CODE))) {
                return actionKey.getString(ACTION_VALUE);
            }
        }
        return null;
    }
}

5. Unified Business Logic and Single Level of Abstraction

The Single Level of Abstraction Principle [7] is a concept proposed by Neal Ford (ThoughtWorks Director-Level Consultant) in The Productive Programmer [8]. SLAP emphasizes that all code in each method is at the same level of abstraction. If high-level abstractions and underlying details are mixed, the code will appear messy and difficult to be understood, resulting in complexity.

public List<XSpaceCaseDTO> queryCaseList(String aliId, int currentPage, int pageSize) {
    // Obtain the ticket list information from xspace.
    List<CaseTaskRelatedDO> caseTaskInfoList = queryCaseListFromXspace(aliId, currentPage, pageSize);
    // Obtain the status details of each ticket.
    Map<Long, CaseActionInfo> caseId2ActionInfoMap = queryCaseId2ActionMap(caseTaskRelatedList);
    // Assemble the ticket data information.
    List<XSpaceCaseDTO> xSpaceCaseList = caseTaskConvertor.convert(caseTaskInfoList, caseId2ActionInfoMap);
    return xSpaceCaseList;
}

Good Smell

In the end, the logic of the refactored code body is listed below. We maintain the readability and maintainability of the code while ensuring the availability of the program.

private CaseActionInfo queryResolveAction(Long caseId) {
    // Obtain the ticket status collection.
    Map<Integer, ActionDTO> actionMap = queryCaseActionMap(caseId);
    if(actionMap == null){
        return null;
    }
    // Prioritize the "End" status.
    if(actionMap.containsKey(CaseCodeConstant.COMPLETE_ACTION_CODE)){
        ActionDTO completeAction = actionMap.get(CaseCodeConstant.COMPLETE_ACTION_CODE);
        String completeConsequence = CaseParseUtils.getCompleteConsequence(completeAction);
        return buildCaseActionInfo(CaseCodeConstant.CASE_COMPLETED, completeAction.getOperatorNick(), completeAction.getGmtModified(), completeConsequence);
    }
    // Then, judge the status "in contact".
    if(actionMap.containsKey(CaseCodeConstant.CONTACTED_ACTION_CODE)){
        ActionDTO contactAction = actionMap.get(CaseCodeConstant.CONTACTED_ACTION_CODE);
        String contactConsequence = CaseParseUtils.getContactedConsequence(contactAction);
        return buildCaseActionInfo(CaseCodeConstant.CASE_CONTACTED, contactAction.getOperatorNick(), contactAction.getGmtModified(), contactConsequence);
    }
    return CaseActionInfo.emptyAction;
}

Smell Battle

Let's look at the final code effect comparison. Code refactoring does not need a single complex module. It should be in daily development and our coding habit.