记一次代码重构

7次阅读

共计 10966 个字符,预计需要花费 28 分钟才能阅读完成。

单一职责

功能单一

功能单一是 SRP 最基本要求,也就是你一个类的功能职责要单一,这样内聚性才高。

比如,下面这个参数类,是用来查询网站 Buyer 信息的,按照 SRP,里面就应该放置查询相关的 Field 就好了。

@Data
public class BuyerInfoParam {
    // Required Param
    private Long buyerCompanyId;
    private Long buyerAccountId;
    private Long callerCompanyId;
    private Long callerAccountId;

    private String tenantId;
    private String bizCode;
    private String channel; // 这个 Channel 在查询中不起任何作用,不应该放在这里
}

可是呢?事实并不是这样,下面的三个参数其实查询时根本用不到,而是在组装查询结果的时候用到,这给我阅读代码带来了很大的困惑,因为我一直以为这个 channel(客户来源渠道)是一个查询需要的一个重要信息。

那么如果和查询无关,为什么要把它放到查询 param 里面呢,问了才知道,只是为了组装查询结果时拿到数据而已。

所以我重构的时候,果断把查询没用到的参数从 BuyerInfoParam 中移除了,这样就消除了理解上的歧义。

Tips:不要为了图方便,而破坏 SOLID 原则,方便的后果就是代码腐化,看不懂,往后要付出的代价更高。

功能内聚

在类的职责单一基础之上,我们还要识别出是不是有功能相似的类或者组件,如果有,是不是要整合起来,而不要让功能类似的代码散落在多处。

比如,代码中我们有一个 TenantContext,而 build 这个 Context 统一是在 ContextPreInterceptor 中做的,其中 Operator 的值一开始只有 crmId,但是随着业务的变化 operator 的值在不同的场景值会不一样,可能是 aliId,也可能是 accountId。

这样就需要其它 id 要转换成 crmId 的工作,重构前这个转换工作是分散在多个地方,不满足 SRP。

    // 在 BaseMtopServiceImpl 中有 crmId 转换的逻辑
    public String getCrmUserId(Long userId){AccountInfoDO accountInfoDO = accountQueryTunnel.getAccountDetailByAccountId(userId.toString(), AccountTypeEnum.INTL.getType(), false);
        if(accountInfoDO != null){return accountInfoDO.getCrmUserId();
        }
        return StringUtils.EMPTY;
    }

    // 在 ContextUtilServiceImpl 中有 crmId 转换的逻辑
    public String getCrmUserIdByMemberSeq(String memberSeq) {if(StringUtil.isBlank(memberSeq)){return null;}
        MemberMappingDO mappingDO = memberMappingQueryTunnel.getMappingByAccountId(Long.valueOf(memberSeq));
        if(mappingDO == null || mappingDO.getAliId() == null){return null;}
    }

重构的做法是将 build context 的逻辑,包括前序的 crmId 的转换逻辑,全部收拢到 ContextPreInterceptor,因为它的职责就是 build context,这样代码的内聚性,可复用性和可读性都会好很多。

    @Override
    public void preIntercept(Command command) {
        ...
        String crmUserId = getCrmUserId(command);
        if(StringUtil.isBlank(crmUserId)){throw new BizException(ErrorCode.BIZ_ERROR_USER_ID_NULL, "can not find crmUserId with"+command.getOperater());
        }
        ...
    }

    // 将 crmId 的转换逻辑收拢至此
    private String getCrmUserId(Command command) {if(command.getOperatorIdType() == OperatorIdType.CRM_ID){return command.getOperater();
        }
        else if(command.getOperatorIdType() == OperatorIdType.ACCOUNT_ID){MemberMappingDO mappingDO = memberMappingQueryTunnel.getMappingByAccountId(Long.valueOf(command.getOperater()));
            if(mappingDO == null || mappingDO.getAliId() == null){return null;}
            return fetchByAliId(mappingDO.getAliId().toString());

        }
        else if(command.getOperatorIdType() == OperatorIdType.ALI_ID){return fetchByAliId(command.getOperater());
        }
        return null;
    }

开闭原则

OCP 是 OO 非常重要的原则,遵循 OCP 可以极大的提升我们代码的扩展性,要想写出好的 OCP 代码,前提是要熟练掌握常用的设计模式,常用的有助于 OCP 的设计模式有 Abstract Factory,Template,Strategy,Chain of Responsibility, Obeserver, Decorator 等

关于 OCP 的重构需要注意两点:

  • 不要滥用设计模式:不要有了锤子就到处乱锤,不恰当的使用不解决问题不说,还会增加复杂度。
  • 要敢于重构:很多的功能点一开始都不需要扩展的,但是随着业务的发展,会变得需要扩展,此时就需要果敢一点,该重构重构,该上设计模式,上,不能等,不能将就!

这里主要以 Template 和 Chain of Responsibility 为例,来介绍关于 OCP 的重构。

模板方法

比如,在处理 Leads 的时候,针对不同的 Leads 来源,其处理可能稍有不同,所以我重构的时候,觉得模板方法是比较好的选项。

因为对于不同的 Leads 来源,只有在线索已存在,但没创建过客户的情况下,处理逻辑不同,其它逻辑都可以共用,那么把 processRepeatedLeadsWithNoCustomer 设置为 abstract 就好了。

Tips:在使用设计模式的时候,最好能在类的命名上体现这个设计模式,这样看代码的人会更容易理解。

public abstract class AbstractLeadsProcessTemplate implements LeadsProcessServiceI {
  ...
   @Override
    public void processLeads(IcbuLeadsE icbuLeadsE) {IcbuLeadsE existingLeads = icbuLeadsE.checkRepeat();
        //1. 新线索
        if(existingLeads == null){processBrandNewLeads(icbuLeadsE);
        }
        // 2\. 线索已经存在,但是没有创建过客户
        else if(existingLeads.isCustomerNotCreated()){processRepeatedLeadsWithNoCustomer(existingLeads, icbuLeadsE);
        }
        //3\. 线索已经存在,且创建过客户,尝试捡回私海
        else{processRepeatedLeadsAndCustomer(existingLeads, icbuLeadsE);
        }
    }

    /**
     * 处理线索已存在,客户未创建
     * @param existingLeads 系统中已存在的 Leads
     * @param comingLeads  新来的 Leads
     */
    public abstract void processRepeatedLeadsWithNoCustomer(IcbuLeadsE existingLeads, IcbuLeadsE comingLeads);

责任链

在我们的营销系统中,有一个 EDM(Email Direct Marketing)的功能,在发送邮件之前,我们要根据规则过滤掉一些客户,比如没有邮箱地址,没有订阅关系,3 天内重复发送等等,而且这个规则随着业务的变化,很可能会增加或减少。

这里用 if-else 当然也能解决问题,但很明显不满足 OCP,如果用一个 Pipeline 的模式,其扩展性就会好很多,类似于 Servlet API 中的FilterChain,增加、删除 Filter 都很灵活。

FilterChain filterChain = FilterChainFactory.buildFilterChain(
            NoEmailAddressFilter.class, 
            EmailUnsubscribeFilter.class, 
            EmailThreeDayNotRepeatFilter.class);

// 具体的 Filter
public class NoEmailAddressFilter implements Filter {
    @Override
    public void doFilter(Object context, FilterInvoker nextFilter) {Map<String,  Object> contextMap = (Map<String,  Object>)context;
        String email = ConvertUtils.convertParamType(contextMap.get("email"), String.class);
        if(StringUtils.isBlank(email)){contextMap.put("success", false);
            return;
        }
        nextFilter.invoke(context);
    }
}

SOFA 框架

这次代码重构中发现,对于框架概念的误用,也是造成代码混乱的原因之一,在 SOFA 框架中我们明确定义了 module,package 和 class 的 scope 和功能,但是在实施过程中,还是出现了在层次归属,命名和使用上的一些混乱,特别是 Convertor,Assembler 和扩展点。

Convertor

在 SOFA 中有三类主要的对象:

  1. ClientObject: 也就是二方库中的数据对象,主要承担 DTO 的职责。
  2. Entity/ValueObject: 也就是既有属性又有行为的领域实体。
  3. DataObeject:是用来获取数据用的,主要是 DAO 使用。

而 Convertor 在上面三个对象之间的转换起到了至关重要的作用,然而 Convertor 里面的逻辑应该是简单的,大部分都是 setter/getter, 如果属性重复度很高的话,也可以使用BeanUtils.copyProperties 让代码变得更简洁。

但事实情况是,现在系统中很多的 Convertor 逻辑并没有在 Convertor 里面。

比如将询盘数据 convert 成 LeadsE,处理类是LeadsBuildStrategy,这个命名是不合适的。

所以我将这段逻辑重构到 ICBULeadsConvertor 也等于是在清晰的告诉看代码的人,这里是做了一个 Client 数据到 LeadsEntity 的转换,仅此而已。

Assembler

Assembler 在 SOFA 框架中的定义是组装参数使用的,所以看到 Assembler 我就很清楚这个类是用来组装参数的。

例如,组装 OpenSearch 的查询条件,原来用的是扩展点 CustomerRepeatRuleExtPt
但是 RuleExt 这个概念,只有在不同业务场景下的业务扩展才需要用到,所以这种命名和使用是不恰当的,组装参数直接用 RepeatCheckConditionAssembler 就好了。

重构前:

List<RepeatConditionDO> repeatConditions = 
                ruleExecutor.execute(CustomerRepeatRuleExtPt.class, 
                repeatExt -> repeatExt.getRepeatCondition(queryField));

重构后:

 RepeatConditionDO repeatConditionDO = 
 repeatCheckConditionAssembler.assembleCustomerRepeatCheckCondition(repeatCheckParam);

扩展点

扩展点是我 SOFA 框架中针对不同业务或租户的一种扩展机制,现在代码中有一些使用,但是因为我们目前只有 ICBU 的场景,所以基本上所有的扩展点只有一个扩展实现。

如果这样的话,我建议先不要提前抽出来扩展点,等有多业务场景的时候,再去重构。

例如 OppotunityDistributeRuleExtPtOpportunityOrgChangeRuleExtPtLeadsCreateCustomerRuleExtPtCustomerNoteAppendRuleExtPt 等等,这样的 case 有很多。

如果是业务内的扩展,使用 Strategy Pattern 就好了。

Tips:简单一点,敏捷一点,不要太多的“提前设计和规划”,等变化来了再重构,Keep it Simple!

领域建模

领域建模的核心,是在深入理解业务的基础上,进行合理的领域抽象,将重要的业务知识、领域概念用通用语言(Ubiquitous Langua)的方式,统一的在 PRD,模型和代码中进行显性化的表达,从而提升代码的可读性和可维护性。

所以能否合理的抽象出 Value Object,Domain Entity,领域行为和领域服务,将成为 DDD 运用是否得当的关键。

Tips:错误的抽象,随心而欲的乱抽象,还不如不要抽象。

领域对象

领域对象主要包括 ValueObject 和 Entity,二者都是在表达一个重要的领域概念,其区别在于 ValueObject 是 immutable 的,而 Entity 不是,它需要有唯一的 id 做标识。

在进行领域对象抽取的时候,要注意以下两点:

1、不要把重要的领域概念遗弃在 Domain 外面:

比如这次重构的主要业务——Leads 引入,原来的 Leads Entity 形同虚设,业务逻辑都散落在外面,像 Leads 判重Leads 生成客户 等业务知识都没有在 Entity 上体现出来,所以这种建模就是不合理的,也违背了 DDD 的初衷。

2、不要把非领域概念的对象强加到 Domain 中:

比如 RepeatQueryFieldV 只是一个 Search 的查询参数,不应该是一个 ValueObject,更合适的做法是定义成一个 RepeatCheckParam 放到 Infrastructure 层去,这样理解起来更方便。

分析领域

客户通的 Leads 来源很大部分来自于网站的询盘(inquiry)联系人,所以对于新的询盘,我们当成新的 Leads 来处理。但是网站那边的询盘联系人修改呢,这个很明显是 Contact 域的事情,如果你和 Leads 揉在一起,会导致混乱。

   public static LeadsEntryEvent ofAction(String action, LeadsE leads) {
        LeadsEntryEvent event = null;
        LeadsEntryAction entryAction = LeadsEntryAction.of(action);
        if (null != entryAction) {switch (entryAction) {
                case ADD:
                    event = new LeadsAddEvent(leads);
                    break;
                case UPDATE://TODO: This is not Leads, 是联系人,不要放在 Leads 里处理
                    event = new LeadsUpdateEvent(leads);
                    break;
                case DELETE://TODO: This is not Leads, 是联系人,不要放在 Leads 里处理
                    event = new LeadsDeleteEvent(leads);
                    break;
                case ASSIGN://TODO: This is not Leads, 不要放在 Leads 里处理
                    event = new LeadsAssignEvent(leads);
                    break;
                case SYNC://TODO: to delete
                    event = new LeadsSyncEvent(leads);
                    break;
                case IM_ADD:
                    event = new LeadsImChannelAddEvent(leads);
                    break;
                case MC_ADD:
                    event = new LeadsMcChannelAddEvent(leads);
                    break;
                default:

很多的行为,放在多个 Domain 上都可以做,这个时候就要仔细分析,多动动头脑,想想哪个 Domain 是最合适的 Domain,而不是戴着耳机,瞧着代码,实现业务功能,完事走人,留下满地的卫生纸!

领域行为 vs. 领域服务

区分领域行为和领域服务,可以参考下面的划分:

  • 领域行为:是我这个 Entity 范畴之内的,添加到 Entity 身上,千万不要不假思索的就抽成一个 DomainService,这样 Entity 很容易被架空。
  • 领域服务:不是一个 Entity 能 handle 了的行为,可以考虑抽到更上层的 DomainService 中去。

因此,对于增加新 Leads 这个动作来说,直觉上应该是属于 LeadsEntity 的,但是仔细分析一下,我们会发现在增加新 Leads 的同时,还要创建客户、联系人。如果有分配需要的话,还要将机会分配到业务员的私海。

这么多的逻辑,这么多 Entity 的交互,如果再放到 LeadsEntity 就不合适了,所以重构的时候,我抽象出 LeadsProcessService 这个 DomainService,这样在表达上会更加清晰,同事扩展起来也更方便。

   @Override
    public void processLeads(IcbuLeadsE icbuLeadsE) {IcbuLeadsE existingLeads = icbuLeadsE.checkRepeat();
        //1. 新线索
        if(existingLeads == null){processBrandNewLeads(icbuLeadsE);
        }
        // 2\. 线索已经存在,但是没有创建过客户
        else if(existingLeads.isCustomerNotCreated()){processRepeatedLeadsWithNoCustomer(existingLeads, icbuLeadsE);
        }
        //3\. 线索已经存在,且创建过客户,尝试捡回私海
        else{processRepeatedLeadsAndCustomer(existingLeads, icbuLeadsE);
        }
    }

统一语言

PRD 中的语言,我们平时沟通的语言,模型中的语言和我们的代码中的语言要保持一致,如果不一致,或者缺失都会极大的增加我们的代码理解成本,使得代码维护变得困难。

比如 客户判重,联系人判重 都是非常重要的领域概念,但是在我们领域模型上并没有被体现,应该将其凸显出来:

    /**
     * 客户判重
     *
     * @return 如果有重复的客户,返回其 customerId, 否则返回 null
     */
    public String checkRepeat() {RepeatCheckParam repeatCheckParam = RepeatCheckParam.ofCompanyName(companyName);
        ...
    }

    /**
     * 联系人判重,主要通过 email 来判重
     * @return 如果有重复的联系人,返回其 contactId, 否则返回 null
     */
    public String checkRepeat(){if(email == null || email.size() == 0){return null;}
        // 只检查第一个 email, 因为 icbu 业务线中一个联系人只有一个 email
        RepeatCheckParam repeatCheckParam = RepeatCheckParam.ofContactEmail(email.iterator().next());

        ...
    }

在系统中目前还有 checkConflict 表示判重,这个需要团队达成一致,如果大家都同意用 checkRepeat 表示判重,那以后就统一用checkRepeat

这地方不要纠结翻译的准确性什么的,就像公海这个概念我们用的是 PublicSea,这是一个很明显的 chinglish,但是大家读起来顺口,也容易理解,我觉得对于中国程序员来说就比SharedTerritory 要好。

业务语义显性化

还是那句话,代码是写给人读的,机器能执行的代码谁都可以写,写出人能读懂的代码才是 NB。

下面以两个重构案例来说明什么是业务语义显性化,大家自己体会一下差别:

1、判断 Leads 是否创建过客户,其逻辑是看 Leads 是否有 CustomerId

重构前:

if (StringUtil.isBlank(existLeads.getCustomerId())

重构后:

if(existingLeads.isCustomerNotCreated())

Tips:虽然只是一行代码,但是却是编程认知的差别。

2、判断 Customer 是否在自己的私海

重构前:

    public boolean checkPrivate(CustomerE customer) {IcbuCustomerE icbuCustomer = (IcbuCustomerE)customer;
        return !PvgContext.getCrmUserId().equals(NIL_VALUE)
            && icbuCustomer.getCustomerGroup() != CustomerGroup.AliCrmCustomerGroup.CANCEL_GROUP;}

重构后:

   /**
     * 判断是否可以被捡入私海
     * @return
     */
    public boolean isAvailableForPrivateSea(){
        // 如果不是业务员操作,不能捡入私海
        if(StringUtil.isBlank(PvgContext.getCrmUserId())){return false;}
        // 如果是 "删除分组" 的客户,不捡入私海,放到公海
        if(this.getCustomerGroup() == CustomerGroup.AliCrmCustomerGroup.CANCEL_GROUP){return false;}
        return true;
    }

性能优化

B 类的服务压力相对没有那么大,性能往往不是瓶颈,但是并不意味着可以随便挥霍。

比如,Leads 更新操作中,发现一个更新,只需要更新一个字段,但是使用的是全字段更新(有几十个字段),明显的浪费资源,所以新增了一个单字段更新的 SQL。

    // 重构前用这个
    <update id="update" parameterType="CrmLeadsDO">
        UPDATE crm_leads SET GMT_MODIFIED = now()
        <if test="modifier != null">
            , modifier = #{modifier}
        </if>
        <if test="isDeleted != null">
            , is_deleted = #{isDeleted}
        </if>
        <if test="customerId != null">
            , customer_id = #{customerId}
        </if>
        <if test="referenceUserId != null">
            , reference_user_id = #{referenceUserId}
        </if>

        ... 此处省略 N 多字段

        <if test="bizCode != null">
            , biz_code = #{bizCode}
        </if>
        where id = #{id} and tenant_id = #{tenantId} and IS_DELETED = 'n'
    </update>

    // 重构后用这个
    <update id="updateCustomerId" parameterType="CrmLeadsDO">
        UPDATE crm_leads SET GMT_MODIFIED = now()
        <if test="customerId != null">
            , customer_id = #{customerId}
        </if>
        where id = #{id} and tenant_id = #{tenantId} and IS_DELETED = 'n'
    </update>

Tips:性能优化要扣细节,不要一提到性能,就上 ThreadPool。

测试代码

看了大家的测试代码,大部分都写的很乱,没有结构化。

实际上,测试代码很容易结构化的,就是三个步骤:

  1. Prepare:准备数据,包括准备请求参数和数据清理
  2. Execute:执行要测试的代码
  3. Check:校验执行结果

下面是我写的创建 Leads,但是客户没有创建过的测试用例:

    @Test
    public void testLeadsExistWithNoCustomer(){
        //1\. Prepare
        IcbuLeadsAddCmd cmd = getIcbuIMLeadsAddCmd();
        String tenantId = "cn_center_10002404";
        LeadsE leadsE = leadsRepository.getLeads(tenantId, cmd.getContactId());
        if(leadsE != null) {leadsE.setCustomerId(null);
            leadsRepository.updateCustomerId(leadsE);
        }

        //2\. Execute
        commandBus.send(cmd);

        //3\. Check
        leadsE = leadsRepository.getLeads(tenantId, cmd.getContactId());
        Assert.assertEquals(leadsE.getCustomerId(), "179001");
    }

测试代码允许有重复,因为这样可以做到更好的隔离,不至于改了一个数据,影响到其他的测试用例。

Tips:PandoraBoot 启动很慢,如果不是全 mock 的话,建议使用我写的 TestContainer,这样可以提效很多。


本文作者:ali-frank

阅读原文

本文为云栖社区原创内容,未经允许不得转载。

正文完
 0