ReaderMX.
阿里开发者

案例分析｜如何消除代码坏味道

阿王耀兴(承録)约 6 分钟


一、背景

开发一款Idea插件，实现对yaml文件的定制化格式检查。
  • !! 后指定的类路径是否准确
  • yaml中的key是否equal类中field的name
  • value是否能够转换成类中field的类型
  • ……



完成代码功能上线后，使用过程发现很多问题。后在主管帮助下，对代码进行了重构。事后对重构前后代码的好坏进行分析总结，文章下面将从 结构设计 、 代码可读性 、 鲁棒性 3个角度对重构前后代码作比较。

二、代码比较

1 结构设计
before：



after：

比较： after：增加抽象类中的 celtVisitMapping 层代码，对多个代码检查模块做统一代理。做了错误的捕获，后面也可以做一些其他的统一处理（日志、标识参数等），方便拓展。
2 代码可读性

2.1命名

一个好的命名能输出更多的信息，它会告诉你，它为什么存在，它是做什么事的，应该怎么使用。
2.1.1 类
功能 时间 类名称
检查yaml文件是否可以成功反序列化成项目中的对象。 before YamlBaseInspection
after CeltClassInspection
比较：

类的命名要做到见名知意，before的命名 YamlBaseInspection 做不到这一点，通过类名并不能够获取到有用的信息。对于 CeltClassInspection 的命名格式，在了解插件功能的基础上，可以直接判断出属于yaml类格式检查。

2.1.2 函数
功能 时间 函数名称
比较value是否可以反序列化成PsiClass before compareNameAndValue
after compareKeyAndValue
比较： before： 1.name是Class中field中的name，通过函数名称并不能够看出，函数名传达 信息不准确 。 2.Value是yaml中map的概念前后 单位不统一 。两者放在一起，会使阅读代码者很迷惑。 after：函数名前后单位统一，key和Value是一个yaml中map的两个概念。能从函数名得出函数功能：检验Key和Value的是否准确。
2.1.3 变量 
//beforeASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);String className = node.getText().substring(2);//afterASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);String tagClassName = node.getText().substring(2);
比较：
String className 来源可以有两个： 1.通过yaml中 tag 标签在项目中查找得到。 2.PsiClass中的变量类型得出。

after：通过变量名 tagClass 可以快速准确的获取变量名属于上述来源中的第一个，能够降低 阅读代码的复杂度 。变量名可以传递更多有用的信息。

2.2 注释

2.2.1 注释格式
  • before 1.无注释 2.有注释不符合规范
  • after 有注释符合JavaDoc规范 
//beforeprivate boolean checkSimpleValue(PsiClass psiClass, PsiElement value)/** * 检查枚举类的value * @return */boolean checkEnum(PsiClass psiClass,String text)//after/** * @param psiClass * @param value * @return true 正常；false 异常 */private boolean checkSimpleValue(PsiClass psiClass, PsiElement value, ProblemsHolder holder)
2.2.2 注释位置
before： 
//simple类型，检查keyName 和 value格式if (PsiClassUtil.isSimpleType(psiClass)) {//泛型（T）、Object、白名单：不进行检查} else if (PsiClassUtil.isGenericType(psiClass)) {//complex类型} else {}
after： 
// simpleValue 为 null 或者 "null"if (YamlUtil.isNull(value)) {}if (PsiClassUtil.isSimpleType(psiClass)) {    // simple类型，检查keyName 和 value格式    checkSimpleValue(psiClass, value, holder);} else if (PsiClassUtil.isGenericType(psiClass)) {    //泛型（T）、Object、白名单：不进行检查} else {    checkComplexValue(psiClass, value, holder);}
行内注释应该在解释的代码块内。

2.3 方法抽象

before: 
public void compareNameAndValue(PsiClass psiClass, YAMLValue value) {    //simple类型，检查keyName 和 value格式    if (PsiClassUtil.isSimpleType(psiClass)) {    //泛型（T）、Object、白名单：不进行检查    } else if (PsiClassUtil.isGenericType(psiClass)) {    //complex类型    } else {        Map<String, PsiType> map = new HashMap<>();        Map<YAMLKeyValue, PsiType> keyValuePsiTypeMap = new HashMap<>();        //init Map<KeyValue,PsiType>, 注册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(), "找不到这个属性", ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);            }        }        keyValuePsiTypeMap.forEach((yamlKeyValue, psiType) -> {            //todo:数组类型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) {    // simpleValue 为 null 或者 "null"    if (YamlUtil.isNull(value)) {        return;    }    if (PsiClassUtil.isSimpleType(psiClass)) {        // simple类型，检查keyName 和 value格式        checkSimpleValue(psiClass, value, holder);    } else if (PsiClassUtil.isGenericType(psiClass)) {        //泛型（T）、Object、白名单：不进行检查    } else {        checkComplexValue(psiClass, value, holder);    }}boolean checkComplexValue();
比较： before: compareNameAndValue方法代码 过长 ，一个屏幕不能浏览整个方法。方法的框架不能够简洁明亮，即要负责判断类型，进行分发处理，还需要负责complex类型的比较，功能耦合。 after：把对complex对象的比较抽离出一个方法，该方法负责进行复杂类型的比较。原方法只负责区分类型，并调用实际的方法比较。能够清晰的看出方法架构，代码后期易维护。

2.4 if复杂判断

before

after

比较：
before：代码中使用复杂的if嵌套，if是造成阅读代码困难的最重要因素之一。if和for循环的嵌套 深V嵌套 ，代码逻辑不清晰，代码维护比较高，拓展复杂。 after：减少了if嵌套，代码理解成本低，代码 易维护，易拓展 。
3.鲁棒性

3.1 报错信息精准 

//beforeholder.registerProblem(value, "类型无法转换", ProblemHighlightType.GENERIC_ERROR);//afterString errorMsg = String.format("cannot find field:%s in class:%s", yamlKeyValue.getName(), psiClass.getQualifiedName());holder.registerProblem(yamlKeyValue.getKey(), errorMsg, ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);
比较： before：对于格式检查出的错误提示很随意，只说明了 类型无法转换 ， from 是什么？ to 是什么？都没有说明白，很多有用的信息并没有反馈到用户。用户使用体验会比较 差 ，像是一个完全不成熟的产品。 after：提示无法在class中找到某一个field。并且明确说明了是哪一个field，哪一个class。帮组用户及时准确定位错误并解决。

3.2 代码健壮性（异常处理）

空指针
before： 代码需要考虑异常（空指针、预期之外的场景），下面代码有空指针异常，deleteSqlList可能为null，3行调用会抛出NPE，程序没有捕获处理。 
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： 
@Overridepublic void doVisitMapping(@NotNull YAMLMapping mapping, @NotNull ProblemsHolder holder) {    ASTNode node = mapping.getNode().findChildByType(YAMLTokenTypes.TAG);    //取出node    if (YamlUtil.isNull(node)) {        return;    }    if (node.getText() == null || !node.getText().startsWith("!!")) {        // throw new RuntimeException("yaml插件监测异常，YAMLQuotedTextImpl text is null或者不是!!开头");        holder.registerProblem(node.getPsi(), "yaml插件监测异常，YAMLQuotedTextImpl text is null或者不是!!开头", 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);    }}
每一步操作都会考虑异常情况，7、11、20行都有对空指针异常的处理。
比较： after：代码对异常场景考虑更全面，tagString格式非法，空指针，数组越界等等情况。代码更健壮。
switch中的default
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, "未识别的className：" +className, ProblemHighlightType.LIKE_UNKNOWN_SYMBOL);        return false;}

比较：

before：代码存在 隐藏逻辑 String类型会走default逻辑不处理，增加 代码理解的难度 。未对非simple类型的default有异常处理。 after：对String类型写到具体case， 暴漏隐藏逻辑 。并对default做异常处理，代码更 健壮 。

KEEP THE THREAD GOING

更多来源文章