How to Eliminate Bad Code Factor all of important steps for solve software code

1. Background

Develop an Idea plug-in to realize the customized format check of the YAML file.

• Whether the class path specified after !! is accurate
• Whether the key in YAML is equal to the name of the field in the class
• Whether the value can be converted to the type of the field in the class

After the code function was launched, many problems were found. The code was refactored with the help of the supervisor. The analysis and summary of the code before and after the refactoring was given. The following article will compare the code before and after refactoring from the perspectives of structural design, code readability, and robustness.

2. Code Comparison

1. Structural Design

Before:

After:

Comparison:

After: Add celtVisitMapping layer code in abstract classes to act as a unified proxy for multiple code check modules. After capturing errors, you can do other unified processing (logs, identification parameters, etc.) to facilitate expansion.

2. Code Readability

2.1 Naming

A good name can output more information. It will tell you why it exists, what it does, and how it should be used.

2.1.1 Class

Comparison:

The naming of a class should be explicit, which the naming YamlBaseInspection of before cannot realize. Useful information cannot be obtained through the class name. For the CeltClassInspection naming format, you can determine that it belongs to the YAML format check based on the understanding of the plug-in function.

2.1.2 Function

Comparison:

Before:

1. Name is the name of the field in the class. The function name does not express inaccurate information.
2. Value is the map in YAML. The concept of value is not the same before and after. The two together will confuse code readers.

After:

The unit before and after the function name is the same. Key and Value are two concepts of the map in YAML. The function features can be obtained from the name: check whether the Key and Value are accurate.

2.1.3 Variables

//before
ASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);
String className = node.getText().substring(2);

//after
ASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);
String tagClassName = node.getText().substring(2);

Comparison:

There can be two sources of String className:

1. Search for the project using tag in YAML.
2. Derive it from the variable type in PsiClass.

After:

The variable name tagClass can be used to quickly and accurately obtain the variable name that belongs to the first of the preceding sources, which can reduce the code complexity. Variable names can offer more useful information.

2.2 Note

2.2.1 Note Format

• Before

1. No notes.
2. There are notes, but they do not conform to the specifications.

• After

Notes that conform to the JavaDoc specifications.

//before

private boolean checkSimpleValue(PsiClass psiClass, PsiElement value)

/**
 * Check the value of the enumeration class.
 * @return
 */
boolean checkEnum(PsiClass psiClass,String text)

//after
/**
 * @param psiClass
 * @param value
* @return true is normal; false is abnormal.
 */
private boolean checkSimpleValue(PsiClass psiClass, PsiElement value, ProblemsHolder holder)

2.2.2 Location of Notes

Before:

// simple type, check the keyName and value formats.
if (PsiClassUtil.isSimpleType(psiClass)) {

// Generic (T), Object, Whitelist: Do not check.
} else if (PsiClassUtil.isGenericType(psiClass)) {

// complex type
} else {

}

After:

// The simpleValue is null or "null".
if (YamlUtil.isNull(value)) {

}
if (PsiClassUtil.isSimpleType(psiClass)) {
    // simple type, check the keyName and value formats.
    checkSimpleValue(psiClass, value, holder);
} else if (PsiClassUtil.isGenericType(psiClass)) {
    // Generic (T), Object, Whitelist: Do not check.
} else {
    checkComplexValue(psiClass, value, holder);
}

Inline notes should be within the explained code block.

2.3 Method Abstraction

Before:

public void compareNameAndValue(PsiClass psiClass, YAMLValue value) {
    // simple type, check the keyName and value formats.
    if (PsiClassUtil.isSimpleType(psiClass)) {
    // Generic (T), Object, Whitelist: Do not check.
    } else if (PsiClassUtil.isGenericType(psiClass)) {
 
    // complex type
    } else {
        Map<String, PsiType> map = new HashMap<>();
        Map<YAMLKeyValue, PsiType> keyValuePsiTypeMap = new HashMap<>();
        // init Map<KeyValue,PsiType>   Register the error of keyName Error.
        PsiField[] allFields = psiClass.getAllFields();
        YAMLMapping mapping = (YAMLMapping) value;
        Collection<YAMLKeyValue> keyValues = mapping.getKeyValues();
        for (PsiField field : allFields) {
            map.put(field.getName(), field.getType());
        }
        for (YAMLKeyValue keyValue : keyValues) {
            if (map.containsKey(keyValue.getName())) {
                keyValuePsiTypeMap.put(keyValue, map.get(keyValue.getName()));
            } else {
                holder.registerProblem(keyValue.getKey(), "This property cannot be found", ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);
            }
        }
        keyValuePsiTypeMap.forEach((yamlKeyValue, psiType) -> {
            // todo: array type check
            if (psiType instanceof PsiArrayType || PsiClassUtil.isCollectionOrMap(PsiTypeUtil.getPsiCLass(psiType, yamlKeyValue))) {
          
            } else {
                compareNameAndValue(PsiTypeUtil.getPsiCLass(psiType, yamlKeyValue), yamlKeyValue.getValue());
            }
        });
    }
}

After:

public void compareKeyAndValue(PsiClass psiClass, YAMLValue value, ProblemsHolder holder) {
    // The simpleValue is null or "null".
    if (YamlUtil.isNull(value)) {
        return;
    }
  
    if (PsiClassUtil.isSimpleType(psiClass)) {
       
        // simple type, check the keyName and value formats.
        checkSimpleValue(psiClass, value, holder);
  
    } else if (PsiClassUtil.isGenericType(psiClass)) {
       
        // Generic (T), Object, Whitelist: Do not check.
    } else {
        checkComplexValue(psiClass, value, holder);
    }
}
boolean checkComplexValue();

Comparison:

Before:

The code of CompareNameAndValue method is too long. One screen cannot view the entire method. The framework of the method cannot be concise, which means it is responsible for judging types, distributing processing, and comparing complex types and coupling functions.

After:

The comparison of complex objects is extracted as a method, which is responsible for the comparison of complex types. The original method is only responsible for distinguishing the types and calling the actual method comparison. The method architecture can be seen, and the code is easy to maintain at a later stage.

2.4 if Complex Judgment

Before:

After:

Comparison:

Before:

Complex if nesting is used in the code. if is one of the most important factors that make the code difficult to read. if and for loops are nested in deep V nesting. The code logic is unclear, the code maintenance is high, and the expansion is complex.

After:

It reduces if nesting, reduces the cost of code understanding, and is easy to maintain and expand.

(3) Robustness

3.1 Accurate Error Report

//before
holder.registerProblem(value   "The type cannot be converted", ProblemHighlightType.GENERIC_ERROR);


//after
String errorMsg = String.format("cannot find field:%s in class:%s", yamlKeyValue.getName(), psiClass.getQualifiedName());

holder.registerProblem(yamlKeyValue.getKey(), errorMsg, ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);

Comparison:

Before:

The error prompt for format checking is casual, only indicating that the type cannot be converted. What is from? What is to? There is no explanation. A lot of useful information has not been fed back to users. The user experience will be poor, like an immature product.

After:

It indicates that a field cannot be found in the class. It clearly states which field and class is. It helps group users locate and solve errors in a timely and accurate manner.

3.2 Code Robustness (Exception Handling)

Null Pointer

Before:

The code needs to consider exceptions (null pointer, unexpected scenarios). The following code has a null pointer exception, deleteSqlList may be null, the 3-line call will throw NPE, and the program does not catch processing.

YAMLKeyValue deleteSqlList = mapping.getKeyValueByKey("deleteSQLList");
YAMLSequence sequence = (YAMLSequence) deleteSqlList.getValue();
List<YAMLSequenceItem> items = sequence.getItems();
for (YAMLSequenceItem item : items) {
    if (!DELETE_SQL_PATTERN.matcher(item.getValue().getText()).find()) {
        holder.registerProblem(item.getValue(), "sql error", ProblemHighlightType.GENERIC_ERROR);
    }
}

After:

@Override
public void doVisitMapping(@NotNull YAMLMapping mapping, @NotNull ProblemsHolder holder) {
    
    ASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);
    
    // Remove the node.
    if (YamlUtil.isNull(node)) {
        return;
    }
  
    if (node.getText() == null || !node.getText().startsWith("!!")) {
        // throw new RuntimeException("yaml plug-in monitoring exceptions, YAMLQuotedTextImpl text is null or not!! Beginning");
        holder.registerProblem(node.getPsi(), "yaml plug-in monitoring exceptions, YAMLQuotedTextImpl text is null or not!! Beginning", ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);
        return;
    }
    String tagClassName = node.getText().substring(2);
    PsiClass[] psiClasses = ProjectService.findPsiClasses(tagClassName, mapping.getProject());
    if (ArrayUtils.isEmpty(psiClasses)) {
        String errorMsg = String.format("cannot find className = %s", tagClassName);
        holder.registerProblem(node.getPsi(), errorMsg, ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);
        return;
    }
   
    if (psiClasses.length == 1) {
        compareKeyAndValue(psiClasses[0], mapping, holder);
    }
}

Exceptions are considered in each step of the operation. Lines 7, 11, and 20 have null pointer exceptions processing.

Comparison:

After:

The code handles exceptions more comprehensively, such as invalid tagString format, null pointer, and out-of-bounds array. The code is more robust.

Default in Switch

Before:

switch (className) {
    case "java.lang.Boolean":
        break;
    case "java.lang.Character":
        break;
    case "java.math.BigDecimal":
        break;
    case "java.util.Date":
            break;
    default:
}

After:

switch (className) {
    case "java.lang.Boolean":
        break;
    case "java.lang.Character":
        break;
    case "java.math.BigDecimal":
        break;
            case "java.util.Date":
    case "java.lang.String":
        return true;
    default:
        holder.registerProblem(value, "Unrecognized className:" + className, ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);
        return false;
}

Comparison:

Before:

If the code has hidden logic, the string type will follow default logic and will not be processed. This makes the code more difficult to be understood. Exceptions are not handled for the default of the non-simple type.