关于代码审查的一些思考

作为一名代码审查员，首先我们已经具备了丰富的代码开发经验，并且对提交的代码工程非常熟悉

代码审查可以发现并纠正代码中的错误、缺陷和不良实践。通过多人对代码进行仔细的检查和讨论，能够发现一些单独开发时难以察觉的问题，从而提高代码的稳定性和可靠性。代码审查是一个持续的过程，通过有效的代码审查，可以提高代码质量、减少错误、促进团队协作和知识共享。同时通过审查他人的代码，我们可以学习新的技术、方法和最佳实践，同时加强彼此之间的沟通和理解。

综述

在审查代码时，我们通常的评审维度主要包括如下几个方面：

1、代码功能是否完善

2、代码结构是否合理，是否符合规范的编码方式，例如国内的话是否符合阿里java开发手册中的代码规范

3、代码是否充分复用，是否具备良好的扩展性

4、代码的命名是否规范，方法命名、变量命名是否体现了其对应的功能含义

5、在性能上是否能够继续优化，如果是O(n^2)的复杂度是否能优化成O(n)的复杂度

6、在内存占用上是否还能进一步压缩

7、在时间和空间二者之间的利弊权衡

8、复杂代码逻辑是否有对应的注释，注释是否清楚

9、代码是否存在安全性问题

--------------------------------------------------------长文预警----------------------------------------------------------

一、先来两个案例看下

案例1：我们看下下面代码如何优化

PowerMockBuilder

public class PowerMockBuilder extends MockitoMockBuilder{

    /**
     * true - field can be mocked
     */
    @Override
    public boolean isMockable(Field field, Type testedClass) {
        boolean openUserCheckDialog = fileTemplateCustomization.isOpenUserCheckDialog();
        boolean isMockable;
        if (openUserCheckDialog) {
            isMockable =  fileTemplateCustomization.getSelectedFieldNameList().contains(field.getName());
        } else {
            isMockable =  isMockableCommonChecks(field, testedClass);
        }
        LOG.debug("field " + field.getType().getCanonicalName() + " " + field.getName() + " is mockable:" + isMockable);
        return isMockable;
    }

}
MockitoMockBuilder
public class  MockitoMockBuilder implements MockBuilder{
    
    /**
     * true - field can be mocked
     */
    @SuppressWarnings("unused")
    @Override
    public boolean isMockable(Field field, Type testedClass) {
        boolean openUserCheckDialog = fileTemplateCustomization.isOpenUserCheckDialog();
        boolean isMockable;
        if (openUserCheckDialog) {
            isMockable = fileTemplateCustomization.getSelectedFieldNameList().contains(field.getName());
        } else {
            isMockable = isMockableCommonChecks(field, testedClass) && (!field.getType().isFinal() || isMockitoMockMakerInlineOn);
        }
        LOG.debug(
            "field " + field.getType().getCanonicalName() + " " + field.getName() + " is mockable:" + isMockable);
        return isMockable;
    }

    /**
     * checks if field in testedClass can be mocked. evaluates conditions common to all currently supported mock frameworks
     * @return true if input field in testedClass can be mocked
     */
    protected boolean isMockableCommonChecks(Field field, Type testedClass) {
        return !field.getType().isPrimitive() && !isWrapperType(field.getType())
                && !field.isOverridden() && !field.getType().isArray() && !field.getType().isEnum()
                && !testSubjectInspector.isNotInjectedInDiClass(field, testedClass) && !isInitInline(field);
    }
}

MockitoMockBuilder

public class  MockitoMockBuilder implements MockBuilder{
    
    /**
     * true - field can be mocked
     */
    @SuppressWarnings("unused")
    @Override
    public boolean isMockable(Field field, Type testedClass) {
        boolean openUserCheckDialog = fileTemplateCustomization.isOpenUserCheckDialog();
        boolean isMockable;
        if (openUserCheckDialog) {
            isMockable = fileTemplateCustomization.getSelectedFieldNameList().contains(field.getName());
        } else {
            isMockable = isMockableCommonChecks(field, testedClass) && (!field.getType().isFinal() || isMockitoMockMakerInlineOn);
        }
        LOG.debug(
            "field " + field.getType().getCanonicalName() + " " + field.getName() + " is mockable:" + isMockable);
        return isMockable;
    }

    /**
     * checks if field in testedClass can be mocked. evaluates conditions common to all currently supported mock frameworks
     * @return true if input field in testedClass can be mocked
     */
    protected boolean isMockableCommonChecks(Field field, Type testedClass) {
        return !field.getType().isPrimitive() && !isWrapperType(field.getType())
                && !field.isOverridden() && !field.getType().isArray() && !field.getType().isEnum()
                && !testSubjectInspector.isNotInjectedInDiClass(field, testedClass) && !isInitInline(field);
    }
}

我们从代码的结构能看出来，PowerMockBuilder继承了MockitoMockBuilder，且isMockable方法很大地方的逻辑是相似的，因此可以从复用的角度触发对其进行优化

优化后

PowerMockBuilder

public class PowerMockBuilder extends MockitoMockBuilder{
    /**
     * true - field can be mocked
     */
    protected boolean isMockableByMockFramework(Field field) {
        return true;
    }
}

MockitoMockBuilder

public class  MockitoMockBuilder implements MockBuilder{
    
    @Override
    public boolean isMockable(Field field, Type testedClass) {
        boolean openUserCheckDialog = fileTemplateCustomization.isOpenUserCheckDialog();
        boolean isMockable;
        if (openUserCheckDialog) {
            isMockable = fileTemplateCustomization.getSelectedFieldNameList().contains(field.getName());
        } else {
            isMockable = isMockableCommonChecks(field, testedClass) && isMockableByMockFramework(field);
        }
        LOG.debug(
            "field " + field.getType().getCanonicalName() + " " + field.getName() + " is mockable:" + isMockable);
        return isMockable;
    }
    
    /**
     *
     * @param field field to mock
     * @return true if field can be mocked in mock framework
     */
    protected boolean isMockableByMockFramework(Field field) {
        return !field.getType().isFinal() || isMockitoMockMakerInlineOn;
    }
    
    /**
     * checks if field in testedClass can be mocked. evaluates conditions common to all currently supported mock frameworks
     * @return true if input field in testedClass can be mocked
     */
    protected boolean isMockableCommonChecks(Field field, Type testedClass) {
        return !field.getType().isPrimitive() && !isWrapperType(field.getType())
                && !field.isOverridden() && !field.getType().isArray() && !field.getType().isEnum()
                && !testSubjectInspector.isNotInjectedInDiClass(field, testedClass) && !isInitInline(field);
    }
}

案例2：下面代码如何优化

/**
 * true - if should stub tested method
 * @param testMethod method being tested
 * @param testedClassFields fields of owner type being tested
 */
@SuppressWarnings("unused")
public boolean shouldStub(Method testMethod, List<Field> testedClassFields) {
    boolean shouldStub = false;
    if (stubMockMethodCallsReturnValues) {
        for (Field testedClassField : testedClassFields) {
            if (isMockable(testedClassField)) {
                LOG.debug("field "+testedClassField.getName()+" type "+testedClassField.getType().getCanonicalName()+" type methods:"+testedClassField.getType().getMethods().size());
                for (Method fieldMethod : testedClassField.getType().getMethods()) {
                    if (fieldMethod.getReturnType() != null && !"void".equals(fieldMethod.getReturnType().getCanonicalName()) && testSubjectInspector.isMethodCalled(fieldMethod, testMethod)) {
                        shouldStub = true;
                        break;
                    }
                }
            }
        }
    }
    LOG.debug("method "+testMethod.getMethodId()+" should be stabbed:"+shouldStub);
    return shouldStub;
}

/**
 * true - if should verify tested method
 * @param testMethod method being tested
 * @param testedClassFields fields of owner type being tested
 */
@SuppressWarnings("unused")
public boolean shouldVerify(Method testMethod, List<Field> testedClassFields) {
    boolean shouldVerity = false;
    if (stubMockMethodCallsReturnValues) {
        for (Field testedClassField : testedClassFields) {
            if (isMockable(testedClassField)) {
                for (Method fieldMethod : testedClassField.getType().getMethods()) {
                    if (fieldMethod.getReturnType() != null && "void".equals(fieldMethod.getReturnType().getCanonicalName()) && testSubjectInspector.isMethodCalled(fieldMethod, testMethod)) {
                        shouldVerity = true;
                        break;
                    }
                }
            }
        }
    }
    LOG.debug("method "+testMethod.getMethodId()+" should be verified:"+shouldVerity);
    return shouldVerity;
}

Method

public class Method { 
    /**
     *
     * true - if method has a return type and not void
     */
    public boolean hasReturn(){
        return returnType != null && !"void".equals(returnType.getName());
    }
}

首先我们看这两方法，其代码复杂度if，for嵌套已经达到了5层；其主体的代码逻辑是很大相似的，因此同样能够复用

优化后

/**
 * true - if should stub tested method
 * @param testMethod method being tested
 * @param testedClassFields fields of owner type being tested
 */
@SuppressWarnings("unused")
public boolean shouldStub(Method testMethod, List<Field> testedClassFields) {
    return callsMockMethod(testMethod, testedClassFields, Method::hasReturn);
}

/**
 * true - if should verify tested method
 * @param testMethod method being tested
 * @param testedClassFields fields of owner type being tested
 */
@SuppressWarnings("unused")
public boolean shouldVerify(Method testMethod, List<Field> testedClassFields) {
    return callsMockMethod(testMethod, testedClassFields, method -> !method.hasReturn());
}

private boolean callsMockMethod(Method testMethod, List<Field> testedClassFields,
    Predicate<Method> mockMethodRelevant) {
    boolean callsMockMethod = false;
    if (!stubMockMethodCallsReturnValues) {
        LOG.debug("method " + testMethod.getMethodId() + " is calling a mock method:" + callsMockMethod);
        return callsMockMethod;
    }
    for (Field testedClassField : testedClassFields) {
        if (!isMockable(testedClassField)) {
            continue;
        }
        LOG.debug("field " + testedClassField.getName() + " type " + testedClassField.getType().getCanonicalName()
            + " type methods:" + testedClassField.getType().getMethods().size());
        for (Method fieldMethod : testedClassField.getType().getMethods()) {
            if (mockMethodRelevant.test(fieldMethod)
                && testSubjectInspector.isMethodCalled(fieldMethod, testMethod)) {
                return true;
            }
        }
    }
    LOG.debug("method " + testMethod.getMethodId() + " is calling a mock method:" + callsMockMethod);
    return callsMockMethod;
}

二、实际国外大佬是如何评审代码的

背景

我将以一个pr请求为例，看看老外如何审查代码，以此进行参考。https://github.com/wrdv/testme-idea/pull/28

此次pr的背景是，给一个开源项目增加一个新特性，在idea中打开一个对话框，可以让用户自行选择他所需要mock的对象和测试的方法。

第一阶段提交

首先，考虑到需要在对话框中先对测试类中的属性和方法先进行一次过滤清洗，我将原先代码中生成类上下文信息的方法提到了前面以便生成对话框时对上下文信息进行复用。

原代码

@Override
    public void invoke(@NotNull final Project project, Editor editor, @NotNull PsiElement element) throws IncorrectOperationException {
        LOG.debug("Start CreateTestMeAction.invoke");
        final Module srcModule = ModuleUtilCore.findModuleForPsiElement(element);
        if (srcModule == null) return;
        final PsiClass srcClass = getContainingClass(element);
        if (srcClass == null) return;
        PsiDirectory srcDir = element.getContainingFile().getContainingDirectory();
        final PsiPackage srcPackage = JavaDirectoryService.getInstance().getPackage(srcDir);

        final PropertiesComponent propertiesComponent = PropertiesComponent.getInstance();
        Module testModule = suggestModuleForTestsReflective(project, srcModule);
        final List<VirtualFile> testRootUrls = computeTestRoots(testModule);
//            final HashSet<VirtualFile> testFolders = new HashSet<VirtualFile>(); //from v14
//        checkForTestRoots(srcModule, testFolders); //from v14
        if (testRootUrls==null|| testRootUrls.isEmpty() && computeSuitableTestRootUrls(testModule).isEmpty()) {
            testModule = srcModule;
            if (!propertiesComponent.getBoolean(CREATE_TEST_IN_THE_SAME_ROOT, false)) {
                if (Messages.showOkCancelDialog(project, "Create test in the same source root?", "No Test Roots Found", Messages.getQuestionIcon()) !=
                        Messages.OK) {
                    return;
                }
                propertiesComponent.setValue(CREATE_TEST_IN_THE_SAME_ROOT, String.valueOf(true));
            }
        }
        final PsiDirectory targetDirectory = targetDirectoryLocator.getOrCreateDirectory(project, srcPackage, testModule);
        if (targetDirectory == null) {
            return;
        }
        LOG.debug("targetDirectory:"+targetDirectory.getVirtualFile().getUrl());
        final ClassNameSelection classNameSelection = generatedClassNameResolver.resolveClassName(project, targetDirectory, srcClass, templateDescriptor);
        if (classNameSelection.getUserDecision() != ClassNameSelection.UserDecision.Abort) {
            final Module finalTestModule = testModule;
            CommandProcessor.getInstance().executeCommand(project, new Runnable() {
                @Override
                public void run() {
                    testMeGenerator.generateTest(
                            new FileTemplateContext(
                                    new FileTemplateDescriptor(templateDescriptor.getFilename()),templateDescriptor.getLanguage(),project, classNameSelection.getClassName(), srcPackage, srcModule, finalTestModule,targetDirectory, srcClass,
                                    new FileTemplateConfig(TestMeConfigPersistent.getInstance().getState())
                            )
                    );
                }
            }, "TestMe Generate Test", this);
        }
        LOG.debug("End CreateTestMeAction.invoke");
    }

修改后核心内容是将testTemplateContextBuilder.build 方法前置了，然后将方法的返回作为参数传给createTestMeDialog，来用作属性和方法过滤的参数

@Override
    public void invoke(@NotNull final Project project, Editor editor, @NotNull PsiElement element) throws IncorrectOperationException {
        LOG.debug("Start CreateTestMeAction.invoke");
        final Module srcModule = ModuleUtilCore.findModuleForPsiElement(element);
        if (srcModule == null) return;
        final PsiClass srcClass = getContainingClass(element);
        if (srcClass == null) return;
        PsiDirectory srcDir = element.getContainingFile().getContainingDirectory();
        final PsiPackage srcPackage = JavaDirectoryService.getInstance().getPackage(srcDir);

        final PropertiesComponent propertiesComponent = PropertiesComponent.getInstance();
        Module testModule = suggestModuleForTestsReflective(project, srcModule);
        final List<VirtualFile> testRootUrls = computeTestRoots(testModule);
//            final HashSet<VirtualFile> testFolders = new HashSet<VirtualFile>(); //from v14
//        checkForTestRoots(srcModule, testFolders); //from v14
        if (testRootUrls==null|| testRootUrls.isEmpty() && computeSuitableTestRootUrls(testModule).isEmpty()) {
            testModule = srcModule;
            if (!propertiesComponent.getBoolean(CREATE_TEST_IN_THE_SAME_ROOT, false)) {
                if (Messages.showOkCancelDialog(project, "Create test in the same source root?", "No Test Roots Found", Messages.getQuestionIcon()) !=
                        Messages.OK) {
                    return;
                }
                propertiesComponent.setValue(CREATE_TEST_IN_THE_SAME_ROOT, String.valueOf(true));
            }
        }
        final PsiDirectory targetDirectory = targetDirectoryLocator.getOrCreateDirectory(project, srcPackage, testModule);
        if (targetDirectory == null) {
            return;
        }
        LOG.debug("targetDirectory:"+targetDirectory.getVirtualFile().getUrl());
        final ClassNameSelection classNameSelection = generatedClassNameResolver.resolveClassName(project, targetDirectory, srcClass, templateDescriptor);
        if (classNameSelection.getUserDecision() == ClassNameSelection.UserDecision.Abort) {
            return;
        }
        final Module finalTestModule = testModule;
        FileTemplateContext fileTemplateContext = new FileTemplateContext(
            new FileTemplateDescriptor(templateDescriptor.getFilename()), templateDescriptor.getLanguage(), project,
            classNameSelection.getClassName(), srcPackage, srcModule, finalTestModule, targetDirectory, srcClass,
            new FileTemplateConfig(TestMeConfigPersistent.getInstance().getState()));

        TestTemplateContextBuilder testTemplateContextBuilder =  new TestTemplateContextBuilder(new MockBuilderFactory(), new MethodReferencesBuilder());
        FileTemplateManager fileTemplateManager = TestMeTemplateManager.getInstance(fileTemplateContext.getTargetDirectory().getProject());
        final Map<String, Object> templateCtxtParams = testTemplateContextBuilder.build(fileTemplateContext, fileTemplateManager.getDefaultProperties());

        boolean openUserCheckDialog = Objects.requireNonNull(TestMeConfigPersistent.getInstance().getState()).isOpenUserCheckDialog();
        if (openUserCheckDialog) {
            // create filed and method check dialog
            final CreateTestMeDialog dialog = createTestMeDialog(project, fileTemplateContext.getSrcClass(),
                fileTemplateContext.getFileTemplateDescriptor().getDisplayName(), templateCtxtParams);
            // if not ok button selected the return
            if (dialog.isModal() && !dialog.showAndGet()) {
                return;
            }
        }

        CommandProcessor.getInstance().executeCommand(project, new Runnable() {
            @Override
            public void run() {
                testMeGenerator.generateTest(fileTemplateContext, templateCtxtParams);
            }
        }, "TestMe Generate Test", this);
        LOG.debug("End CreateTestMeAction.invoke");
    }

在对话框中的initExtractingClassMembers方法中，根据传递过来的上下文templateCtxtParams来对属性和方法过滤

/**
 *
 * dialog for user to check fields mockable and methods testable
 *
 * @author huangliang
 */
public class CreateTestMeDialog extends DialogWrapper {

    private final Map<String, Object> templateCtxtParams;
    private final PsiClass myTargetClass;
    private final String templateFileName;
    private final MemberSelectionTable myMethodsTable = new MemberSelectionTable(Collections.emptyList(), null);
    private final MemberSelectionTable myFieldsTable = new MemberSelectionTable(Collections.emptyList(), null);
    private final List<String> checkedFieldNameList = new ArrayList<>();
    private final List<String> checkedMethodIdList = new ArrayList<>();
    
    public CreateTestMeDialog(@NotNull Project project, @NotNull @NlsContexts.DialogTitle String title,
        PsiClass targetClass, String templateFileName, Map<String, Object> templateCtxtParams) {
        super(project, true);
        myTargetClass = targetClass;
        this.templateCtxtParams = templateCtxtParams;
        this.templateFileName = templateFileName;
        setTitle(title);
        init();
    }

    @Override
    protected JComponent createCenterPanel() {
        JPanel panel = new JPanel();
        panel.setLayout(new BoxLayout(panel, BoxLayout.Y_AXIS));

        JLabel fieldLabel = new JLabel("Select Need Mocked Fields");
        panel.add(fieldLabel);
        panel.add(ScrollPaneFactory.createScrollPane(myFieldsTable));

        JLabel methodLabel = new JLabel("Select Need Test Methods");
        panel.add(methodLabel);
        panel.add(ScrollPaneFactory.createScrollPane(myMethodsTable));

        initExtractingClassMembers();
        return panel;
    }

    /**
     * update field is mockable or not after user checked
     */
    private void updateClassMockableFields() {
        Collection<MemberInfo> selectedMemberInfos = myFieldsTable.getSelectedMemberInfos();
        List<String> userCheckedMockableFieldsList =
            selectedMemberInfos.stream().map(e -> e.getMember().getName()).toList();
        Type classTypeObj = (Type)templateCtxtParams.get(TestMeTemplateParams.TESTED_CLASS);
        for (Field field : classTypeObj.getFields()) {
            if (userCheckedMockableFieldsList.contains(field.getName())) {
                checkedFieldNameList.add(field.getName());
            }
        }
        templateCtxtParams.put(TestMeTemplateParams.USER_CHECKED_MOCK_FIELDS, checkedFieldNameList);
    }

    /**
     * update method is testable or not after user checked
     */
    private void updateClassTestableMethods() {
        Collection<MemberInfo> selectedMemberInfos = myMethodsTable.getSelectedMemberInfos();
        List<String> testableMethodList =
            selectedMemberInfos.stream().map(e -> PsiMethodUtils.formatMethodId((PsiMethod)e.getMember())).toList();
        Type classTypeObj = (Type)templateCtxtParams.get(TestMeTemplateParams.TESTED_CLASS);
        for (Method method : classTypeObj.getMethods()) {
            if (testableMethodList.contains(method.getMethodId())) {
                checkedMethodIdList.add(method.getMethodId());
            }
        }
        templateCtxtParams.put(TestMeTemplateParams.USER_CHECKED_TEST_METHODS, checkedMethodIdList);
    }

    /**
     * init and extract class fields and methods for user to check
     */
    public void initExtractingClassMembers() {
        Set<PsiClass> classes= InheritanceUtil.getSuperClasses(myTargetClass);
        classes.add(myTargetClass);

        Type classTypeObj = (Type)templateCtxtParams.get(TestMeTemplateParams.TESTED_CLASS);
        TestSubjectInspector testSubjectUtil =
            (TestSubjectInspector)templateCtxtParams.get(TestMeTemplateParams.TestSubjectUtils);
        MockBuilder templateMockBuilder = getTemplateMockBuilder(templateFileName);

        // build field mockable map, key = field name, value = true - if field mockable
        Map<String, Boolean> fieldMockableMap = new HashMap<>();
        for (Field field : classTypeObj.getFields()) {
            Boolean fieldMockable = templateMockBuilder.isMockable(field, classTypeObj);
            fieldMockableMap.put(field.getName(), fieldMockable);
        }
        // build method testable map, key = method id, value = true - if method testable
        Map<String, Boolean> methodTestableMap = new HashMap<>();
        for (Method method : classTypeObj.getMethods()) {
            Boolean testable = testSubjectUtil.shouldBeTested(method);
            methodTestableMap.put(method.getMethodId(), testable);
        }

        // init method table and field table
        List<MemberInfo> methodResult = new ArrayList<>();
        List<MemberInfo> fieldResult = new ArrayList<>();
        for (PsiClass aClass : classes) {
            if (CommonClassNames.JAVA_LANG_OBJECT.equals(aClass.getQualifiedName()))
                continue;
            initMethodsTable(aClass, methodResult, methodTestableMap);
            initFieldsTable(aClass, fieldResult, fieldMockableMap);
        }
    }

    /**
     * get mock builder for template
     * @param templateFileName template name
     * @return MockBuilder
     */
    private MockBuilder getTemplateMockBuilder(String templateFileName) {
        if (TemplateRegistry.JUNIT4_POWERMOCK_JAVA_TEMPLATE.equals(templateFileName)) {
            return (PowerMockBuilder)templateCtxtParams.get(TestMeTemplateParams.PowerMockBuilder);
        } else {
            return (MockitoMockBuilder)templateCtxtParams.get(TestMeTemplateParams.MockitoMockBuilder);
        }
    }

    private void initMethodsTable(PsiClass myTargetClass, List<MemberInfo> result, Map<String, Boolean> methodTestableMap) {
        Set<PsiMember> selectedMethods = new HashSet<>();
        MemberInfo.extractClassMembers(myTargetClass, result, member -> {
            if (!(member instanceof PsiMethod method))
                return false;
            String methodId = PsiMethodUtils.formatMethodId(method);
            if (methodTestableMap.containsKey(methodId) && methodTestableMap.get(methodId)) {
                selectedMethods.add(member);
                return true;
            }
            return false;
        }, false);

        for (MemberInfo each : result) {
            each.setChecked(selectedMethods.contains(each.getMember()));
        }

        myMethodsTable.setMemberInfos(result);
    }

    private void initFieldsTable(PsiClass myTargetClass, List<MemberInfo> result, Map<String, Boolean> fieldMockableMap) {
        Set<PsiMember> selectedFields = new HashSet<>();
        MemberInfo.extractClassMembers(myTargetClass, result, member -> {
            if (!(member instanceof PsiField field))
                return false;
            if (fieldMockableMap.containsKey(field.getName()) && fieldMockableMap.get(field.getName())) {
                selectedFields.add(member);
                return true;
            }
            return false;
        }, false);

        for (MemberInfo each : result) {
            each.setChecked(selectedFields.contains(each.getMember()));
        }

        myFieldsTable.setMemberInfos(result);
    }

    @Override
    protected String getDimensionServiceKey() {
        return getClass().getName();
    }

    @Override
    protected void doOKAction() {
        updateClassMockableFields();
        updateClassTestableMethods();
        super.doOKAction();
    }

}

按照这种写法，正常实现了打开对话框并让用户自定义mock的属性和选择测试方法的功能。

第一阶段修改意见

我们看下国外的大佬的修改意见

从交互上考虑，由于构建上文文信息的方法是一个非常重的操作，耗时比较长，如果将上下文构建的方法提前的化，会导致对话框打开延迟时间很长，造成用户体验不好，因此他要求寻找另一种方法来实现属性和方法的初始化。

针对命名的修改

完备性考虑，考虑对空属性列表的问题，他建议如果为空不展示选择属性的区域。

第二阶段提交

按照他的意见，我对代码进行了修改，将上下文信息的初始化还原了，重新修改了属性和方法的初始化过滤方法

/**
 *
 * dialog for user to check fields mockable and methods testable
 *
 * @author huangliang
 */
public class CustomizeTestDialog extends DialogWrapper {

    private final PsiClass myTargetClass;
    private final MemberSelectionTable myMethodsTable = new MemberSelectionTable(Collections.emptyList(), null);
    private final MemberSelectionTable myFieldsTable = new MemberSelectionTable(Collections.emptyList(), null);
    private final FileTemplateContext fileTemplateContext;

    public CustomizeTestDialog(@NotNull Project project, @NotNull @NlsContexts.DialogTitle String title,
        PsiClass targetClass, FileTemplateContext fileTemplateContext) {
        super(project, true);
        myTargetClass = targetClass;
        this.fileTemplateContext = fileTemplateContext;
        setTitle(title);
        init();
    }

    @Override
    protected JComponent createCenterPanel() {
        initExtractingClassMembers();

        JPanel panel = new JPanel();
        panel.setLayout(new BoxLayout(panel, BoxLayout.Y_AXIS));

        if (myFieldsTable.getRowCount() > 0) {
            JLabel fieldLabel = new JLabel("Mock fields:");
            panel.add(fieldLabel);
            panel.add(ScrollPaneFactory.createScrollPane(myFieldsTable));
        }

        JLabel methodLabel = new JLabel("Test Methods:");
        panel.add(methodLabel);
        panel.add(ScrollPaneFactory.createScrollPane(myMethodsTable));

        return panel;
    }

    /**
     * update field is mockable or not after user checked
     */
    private void updateClassMockableFields() {
        Collection<MemberInfo> selectedMemberInfos = myFieldsTable.getSelectedMemberInfos();
        List<String> userCheckedMockableFieldsList =
            selectedMemberInfos.stream().map(e -> e.getMember().getName()).toList();
        fileTemplateContext.getFileTemplateCustomization().getSelectedFieldNameList()
            .addAll(userCheckedMockableFieldsList);
    }

    /**
     * update method is testable or not after user checked
     */
    private void updateClassTestableMethods() {
        Collection<MemberInfo> selectedMemberInfos = myMethodsTable.getSelectedMemberInfos();
        List<String> testableMethodList =
            selectedMemberInfos.stream().map(e -> PsiMethodUtils.formatMethodId((PsiMethod)e.getMember())).toList();
        fileTemplateContext.getFileTemplateCustomization().getSelectedMethodIdList().addAll(testableMethodList);
    }

    /**
     * init and extract class fields and methods for user to check
     */
    public void initExtractingClassMembers() {
        Set<PsiClass> classes;
        if (fileTemplateContext.getFileTemplateConfig().isGenerateTestsForInheritedMethods()) {
            classes = InheritanceUtil.getSuperClasses(myTargetClass);
            classes.add(myTargetClass);
        } else {
            classes = Collections.singleton(myTargetClass);
        }

        // init method table and field table
        List<MemberInfo> methodResult = new ArrayList<>();
        for (PsiClass aClass : classes) {
            if (CommonClassNames.JAVA_LANG_OBJECT.equals(aClass.getQualifiedName()))
                continue;
            initMethodsTable(aClass, methodResult);
        }

        List<MemberInfo> fieldResult = new ArrayList<>();
        initFieldsTable(myTargetClass, fieldResult, fileTemplateContext.getFileTemplateDescriptor().getDisplayName());
    }

    /**
     *
     * @param templateFileName template name
     * @return true - if final can be mocked
     */
    private boolean canMockFinal(String templateFileName) {
        return TemplateRegistry.JUNIT4_POWERMOCK_JAVA_TEMPLATE.equals(templateFileName);
    }

    private void initMethodsTable(PsiClass myTargetClass, List<MemberInfo> result) {
        Set<PsiMember> selectedMethods = new HashSet<>();
        MemberInfo.extractClassMembers(myTargetClass, result, member -> {
            if (!(member instanceof PsiMethod method))
                return false;
            if (shouldBeTested(method, myTargetClass)) {
                selectedMethods.add(member);
                return true;
            }
            return false;
        }, false);

        for (MemberInfo each : result) {
            each.setChecked(selectedMethods.contains(each.getMember()));
        }

        myMethodsTable.setMemberInfos(result);
    }

    private void initFieldsTable(PsiClass myTargetClass, List<MemberInfo> result, String templateFileName) {
        Set<PsiMember> selectedFields = new HashSet<>();
        MemberInfo.extractClassMembers(myTargetClass, result, member -> {
            if (!(member instanceof PsiField field))
                return false;
            if (isMockable(field, myTargetClass, templateFileName)) {
                selectedFields.add(member);
                return true;
            }
            return false;
        }, false);

        for (MemberInfo each : result) {
            each.setChecked(selectedFields.contains(each.getMember()));
        }

        myFieldsTable.setMemberInfos(result);
    }

    @Override
    protected String getDimensionServiceKey() {
        return getClass().getName();
    }

    @Override
    protected void doOKAction() {
        updateClassMockableFields();
        updateClassTestableMethods();
        super.doOKAction();
    }

    public boolean isMockable(PsiField psiField, PsiClass testedClass, String templateFileName) {
        boolean overridden = Field.isOverriddenInChild(psiField, testedClass);
        boolean isFinal =
            psiField.getModifierList() != null && psiField.getModifierList().hasExplicitModifier(PsiModifier.FINAL);
        boolean isPrimitive = psiField.getType() instanceof PsiPrimitiveType;
        PsiClass psiClass = PsiUtil.resolveClassInType(psiField.getType());
        String canonicalText = JavaTypeUtils.resolveCanonicalName(psiClass, null);
        boolean isArray = ClassNameUtils.isArray(canonicalText);
        boolean isEnum = JavaPsiTreeUtils.resolveIfEnum(psiClass);
        boolean isMockitoMockMakerInlineOn = MockBuilderFactory.isMockInline(fileTemplateContext);
        boolean isWrapperType = MockitoMockBuilder.WRAPPER_TYPES.contains(canonicalText);
        return !isPrimitive && !isWrapperType
            && (canMockFinal(templateFileName) || !isFinal || isMockitoMockMakerInlineOn) && !overridden && !isArray
            && !isEnum;
    }

    /**
     * true - method should test
     */
    public boolean shouldBeTested(PsiMethod method, PsiClass psiClass) {
        return MethodFactory.isTestable(method, psiClass);
    }

}

第二阶段修改意见

看下新的修改意见

首先他认为，用户可以自己选择需要测试的方法了，因此可以不在添加生成父类测试方法配置的判断，建议天剑checkbox来让用户自行选择，同时他查看了InheritanceUtil的文档，认为InheritanceUtil.getSuperClasses(myTargetClass, classes, false)这个方法更好，调用这个方法无需进一步剔除非本工程的父类

修改入参的方式不是最佳的实践方式，虽然jetbrain idea 官方源码中是这么写的，但是并不推荐这么修改。

第三阶段修改

public class CustomizeTestDialog extends DialogWrapper {

    private final MemberSelectionTable myMethodsTable;
    private final MemberSelectionTable myFieldsTable;
    private final FileTemplateContext fileTemplateContext;

    public CustomizeTestDialog(@NotNull Project project, @NotNull @NlsContexts.DialogTitle String title,
        PsiClass targetClass, FileTemplateContext fileTemplateContext) {
        super(project, true);
        this.fileTemplateContext = fileTemplateContext;
        myMethodsTable = new MemberSelectionTable(initMethodsTable(targetClass), null);
        myFieldsTable = new MemberSelectionTable(
            initFieldsTable(targetClass, fileTemplateContext.getFileTemplateDescriptor().getDisplayName()), null);
        setTitle(title);
        init();
    }

    @Override
    protected JComponent createCenterPanel() {
        JPanel panel = new JPanel();
        panel.setLayout(new BoxLayout(panel, BoxLayout.Y_AXIS));
        if (myFieldsTable.getRowCount() > 0) {
            JLabel fieldLabel = new JLabel("Mock fields:");
            panel.add(fieldLabel);
            panel.add(ScrollPaneFactory.createScrollPane(myFieldsTable));
        }
        JLabel methodLabel = new JLabel("Test Methods:");
        panel.add(methodLabel);
        panel.add(ScrollPaneFactory.createScrollPane(myMethodsTable));

        return panel;
    }

    /**
     * update field is mockable or not after user checked
     */
    private void updateClassMockableFields() {
        Collection<MemberInfo> selectedMemberInfos = myFieldsTable.getSelectedMemberInfos();
        List<String> userCheckedMockableFieldsList =
            selectedMemberInfos.stream().map(e -> e.getMember().getName()).toList();
        fileTemplateContext.getFileTemplateCustomization().getSelectedFieldNameList()
            .addAll(userCheckedMockableFieldsList);
    }

    /**
     * update method is testable or not after user checked
     */
    private void updateClassTestableMethods() {
        Collection<MemberInfo> selectedMemberInfos = myMethodsTable.getSelectedMemberInfos();
        List<String> testableMethodList =
            selectedMemberInfos.stream().map(e -> PsiMethodUtils.formatMethodId((PsiMethod)e.getMember())).toList();
        fileTemplateContext.getFileTemplateCustomization().getSelectedMethodIdList().addAll(testableMethodList);
    }

    /**
     *
     * @param templateFileName template name
     * @return true - if final can be mocked
     */
    private boolean canMockFinal(String templateFileName) {
        return TemplateRegistry.JUNIT4_POWERMOCK_JAVA_TEMPLATE.equals(templateFileName);
    }

    private List<MemberInfo> initMethodsTable(PsiClass myTargetClass) {
        Set<PsiClass> classes = new HashSet<>();
        InheritanceUtil.getSuperClasses(myTargetClass, classes, false);
        classes.add(myTargetClass);

        Set<PsiMember> selectedMethods = new HashSet<>();
        List<MemberInfo> result = new ArrayList<>();
        // init method table and field table
        for (PsiClass aClass : classes) {
            MemberInfo.extractClassMembers(aClass, result, member -> {
                if (!(member instanceof PsiMethod method))
                    return false;
                if (shouldBeTested(method, myTargetClass)) {
                    selectedMethods.add(member);
                    return true;
                }
                return false;
            }, false);
        }
        for (MemberInfo each : result) {
            each.setChecked(selectedMethods.contains(each.getMember()));
        }
        return result;
    }

    private List<MemberInfo> initFieldsTable(PsiClass myTargetClass, String templateFileName) {
        Set<PsiMember> selectedFields = new HashSet<>();
        List<MemberInfo> result = new ArrayList<>();
        MemberInfo.extractClassMembers(myTargetClass, result, member -> {
            if (!(member instanceof PsiField field))
                return false;
            if (isMockable(field, myTargetClass, templateFileName)) {
                selectedFields.add(member);
                return true;
            }
            return false;
        }, false);

        for (MemberInfo each : result) {
            each.setChecked(selectedFields.contains(each.getMember()));
        }
        return result;
    }

    @Override
    protected String getDimensionServiceKey() {
        return getClass().getName();
    }

    @Override
    protected void doOKAction() {
        updateClassMockableFields();
        updateClassTestableMethods();
        super.doOKAction();
    }

    private boolean isMockable(PsiField psiField, PsiClass testedClass, String templateFileName) {
        boolean isPrimitive = psiField.getType() instanceof PsiPrimitiveType;
        // make a direct return if isPrimitive, because PsiUtil.resolveClassInType(psiField.getType()) returns null
        if (isPrimitive) {
            return false;
        }
        boolean overridden = Field.isOverriddenInChild(psiField, testedClass);
        if (overridden) {
            return false;
        }
        boolean isFinal =
            psiField.getModifierList() != null && psiField.getModifierList().hasExplicitModifier(PsiModifier.FINAL);
        boolean isMockitoMockMakerInlineOn = MockBuilderFactory.isMockInline(fileTemplateContext);
        if (!(canMockFinal(templateFileName) || !isFinal || isMockitoMockMakerInlineOn)) {
            return false;
        }
        PsiClass psiClass = PsiUtil.resolveClassInType(psiField.getType());
        boolean isEnum = JavaPsiTreeUtils.resolveIfEnum(psiClass);
        if (isEnum) {
            return false;
        }
        String canonicalText = JavaTypeUtils.resolveCanonicalName(psiClass, null);
        return (null != canonicalText) && !MockitoMockBuilder.WRAPPER_TYPES.contains(canonicalText)
            && !ClassNameUtils.isArray(canonicalText);
    }

    /**
     * true - method should test
     */
    private boolean shouldBeTested(PsiMethod method, PsiClass psiClass) {
        return MethodFactory.isTestable(method, psiClass);
    }
}

代码修改玩完后更加简洁

三、总结

除了开头简述的那些需要评审的内容外，往往还有许多其他的方面也要考虑。特别是在复杂系统的场景下，如：

1、异常、边界场景是否考虑，以及如何处理

2、是否能够满足特定场景的并发流量，如果涉及分库、分表、缓存等设计是否合理。

3、是否会影响已有的功能、能否向下兼容，如果出现问题如何快速恢复。

等等。这就要求我们平时多思考，多积累对应的经验。

另外打个广告，欢迎大家搜所并下载自动生成单元测试的 jetbrain Idea 开源插件 TestMe，并给我们反馈问题。