記一次程式碼重構

ali-frank發表於2018-08-09

單一職責

功能單一

功能單一是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使用。

image.png

而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的場景，所以基本上所有的擴充套件點只有一個擴充套件實現。

如果這樣的話，我建議先不要提前抽出來擴充套件點，等有多業務場景的時候，再去重構。

例如OppotunityDistributeRuleExtPt、OpportunityOrgChangeRuleExtPt、LeadsCreateCustomerRuleExtPt、CustomerNoteAppendRuleExtPt等等，這樣的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，這樣可以提效很多。


相關文章