記一次代碼重構
單一職責
功能單一
功能單一是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中移除了，這樣就消除了理解上的歧義。app
Tips：不要爲了圖方便，而破壞SOLID原則，方便的後果就是代碼腐化，看不懂，日後要付出的代價更高。框架
功能內聚
在類的職責單一基礎之上，咱們還要識別出是否是有功能類似的類或者組件，若是有，是否是要整合起來，而不要讓功能相似的代碼散落在多處。ide
好比，代碼中咱們有一個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，這樣代碼的內聚性，可複用性和可讀性都會好不少。fetch
@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中有三類主要的對象：
- ClientObject: 也就是二方庫中的數據對象，主要承擔DTO的職責。
- Entity/ValueObject: 也就是既有屬性又有行爲的領域實體。
- 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的場景，因此基本上全部的擴展點只有一個擴展實現。
若是這樣的話，我建議先不要提早抽出來擴展點，等有多業務場景的時候，再去重構。
例如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作標識。
在進行領域對象抽取的時候，要注意如下兩點：
一、不要把重要的領域概念遺棄在Domain外面：
好比此次重構的主要業務——Leads引入，原來的Leads Entity形同虛設，業務邏輯都散落在外面，像Leads判重，Leads生成客戶等業務知識都沒有在Entity上體現出來，因此這種建模就是不合理的，也違背了DDD的初衷。
二、不要把非領域概念的對象強加到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。
下面以兩個重構案例來講明什麼是業務語義顯性化，你們本身體會一下差異：
一、判斷Leads是否建立過客戶，其邏輯是看Leads是否有CustomerId
重構前：
if (StringUtil.isBlank(existLeads.getCustomerId())
重構後：
if(existingLeads.isCustomerNotCreated())
Tips：雖然只是一行代碼，可是倒是編程認知的差異。
二、判斷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。
測試代碼
看了你們的測試代碼，大部分都寫的很亂，沒有結構化。
實際上，測試代碼很容易結構化的，就是三個步驟：
- Prepare：準備數據，包括準備請求參數和數據清理
- Execute：執行要測試的代碼
- 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，這樣能夠提效不少。
原文連接 本文爲雲棲社區原創內容，未經容許不得轉載。
- 1. 記一次代碼重構
- 2. 一次代碼重構記錄
- 3. 記一次.NET代碼重構
- 4. 代碼重構（一）
- 5. 從MVC到MVP,記一次代碼重構
- 6. 記一次用設計模式重構代碼的經驗
- 7. 記一次項目重構
- 8. 記一次篩選重構
- 9. 重構代碼學習筆記一：重構的原則
- 10. Java 代碼重構 (一)
- 更多相關文章...
- • Markdown 代碼 - Markdown 教程
- • Eclipse 代碼模板 - Eclipse 教程
- • IntelliJ IDEA代碼格式化設置
- • Tomcat學習筆記（史上最全tomcat學習筆記）
-
每一个你不满意的现在，都有一个你没有努力的曾经。
- 1. 記一次代碼重構
- 2. 一次代碼重構記錄
- 3. 記一次.NET代碼重構
- 4. 代碼重構（一）
- 5. 從MVC到MVP,記一次代碼重構
- 6. 記一次用設計模式重構代碼的經驗
- 7. 記一次項目重構
- 8. 記一次篩選重構
- 9. 重構代碼學習筆記一：重構的原則
- 10. Java 代碼重構 (一)