记一次代码重构
单一职责
功能单一
功能单一是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 代码重构 (一)