提升代码质量——识别代码之丑

前言

这是一篇读书笔记总结，在我们开发项目中会涉及到代码评审遇见一些有"异味"的代码，便结合自应该怎么识别，识别后如何处理等等提供了一系列的指导与规则，让审核代码有迹可循，提升自己的开发审美与团队代码质量。

一、缺乏业务含义的命名 好的命名，是体现业务含义的命名。 1.1 不精准的命名

int ProcessChapter(int chapter_id) {   
 // 设置为翻译中前置逻辑   
 ……   
 // 设置为翻译中并保存   
 chapter.SetTranslationState(TranslationState::kTranslating);   
 chapter.Save(); 
}

问题：如果说“将章节的翻译状态改成翻译中”叫做处理章节，那么“将章节的翻译状态改成翻译完”是不是也叫处理章节呢？这段代码的问题是命名过于宽泛。

初步优化：ChangeChapterToTranslating，相比上文能描述出代码做了什么，但依然不是好名字，描述的是实现而不是意图。

最终优化：StartTranslation，相比ChangeChapterToTranslating描述了函数的意图。

建议：命名过于宽泛不能精准描述意图，也是代码难以理解的根源所在。

警惕：data、info、flag、process、handle、build、manage、modify……

1.2 使用技术术语命名

List book_list = service.GetBooks();

问题：book_list 变量之所以叫 book_list ，原因就是它声明的类型是 List，如果后期优化为Map呢？这段代码的问题是用技术术语命名，这是程序常犯的错误。

优化：books，与上文相比命名面向意图“拿到了一堆书”。

建议：用业务术语命名而不是用技术术语命名，面向意图命名而不是面向实现。

警惕：List、Map等语言关键字，Mysql、Redies等技术组件名字。

1.3 使用业务语言写代码

void ApproveChapter(int chapter_id, std::string id)

问题：id 是什么id？在业务中实际上是要记录审核人的信息，id并不是好的命名，因为它还需要更多的解释。

优化：reviewer_user_id

建议：好的实践是建立团队的词汇表。

警惕：id、state……

1.4 总结 精准命名，体现业务含义，表明意图，不暴露细节，不使用技术用语。 二、乱用英语 2.1 违反语法规则的命名

void CompletedTranslate(int chapter_id)

问题：CompletedTranslate可以看出作者想表达的是“完成翻译”，但遗憾的是这个名字还是起错了，方法要表示一个动作。

优化：CompleteTranslation表示完成翻译的动作。

建议：类名是一个名词，表示一个对象，而方法名则是一个动词或者是动宾短语，表示一个动作。

警惕：非名词的类名、非动名词/动词的方法名

2.2 不准确的英语词汇

// 实现一个章节审核的功能，审核状态定义
enum ChapterAuditState {    
 PENDING,    
 APPROVED,    
 REJECTED;
}

问题：audit更适合翻译为审计，有官方的味道。

优化：review。

建议：最好的解决方案还是建立起一个业务词汇表，千万不要臆想；去github搜一下，看看别人怎么用的。

警惕：当使用翻译工具得到多个词时应该仔细推敲，例如state和status、audit和review。

2.3 错误的拼写

// 单词排序
class QuerySort {    
 SortFiled sortFiled;    
 ...
}

问题：排序字段，应该sortField。

建议：IntelliJ IDEA、VsCode开启拼写检查，安装拼写检查工具。

三、重复代码 3.1 写真正的if else

if (user.IsEditor()) {  
 service.EditChapter(chapterId, title, content, true);
} else {  
 service.EditChapter(chapterId, title, content, false);
}

问题：if 选择的一定是两段不同的业务处理，但这里if 和 else 两段代码几乎是一模一样，这段代码客观上也造就了重复。

优化：service.EditChapter(chapterId, title, content, user.IsEditor());

建议：不要只想 if 语句判断之后要做什么，而要想到这个 if 语句判断的到底是什么。

警惕：if  else

3.2 重复的结构

// 发送支付成功消息
void SendPaySuccessMessage() {   
 ……   
 // 构造支付成功消息对象   
 ……  
 int ret = SendMessage(message);  
 if(0!= ret) {    
  Report();    
  PLOG_ERR("SendMessage",ret,"message=%s",message.toString());  
 }  
}
// 发送欠款消息
void SendDebtMessage() {   
 ……   
 // 构造欠款消息对象   
 ……  
 int ret = SendMessage(message);  
 if(0!= ret) {    
  Report();    
  PLOG_ERR("SendMessage",ret,"message=%s",message.toString());  
 }  
}

问题：这段代码的调用SendMessage失败处理逻辑重复。

优化：将打印日志和上报逻辑放在SendMessage函数中。

建议：识别相似的结构。

警惕：异常处理、上报等，以及当你使用ctrl+v时。

四、长函数

问题：如果一个函数超过了 40 行，则可以思考下，能否在不破坏程序结构的前提之下，对函数进行拆分。

建议：

1. 不能以性能为由编写长函数，性能优化不应该是写代码的第一考量；
2. 面向意图拆分小函数，不能“平铺直叙”；
3. 让营地比你来时更干净（—— 童子军军规），坚决对抗一次加一点，逐渐糟糕腐坏的代码。

警惕：

1. 当不知道如何命名时很可能就是函数过长需要拆分；
2. 单元测试需要大量的Mock时；
3. 函数中变量名字很长时也可能就是函数过长的结果。

五、大类

问题：关于什么样的类是大类并没有找到明确定义，个人总结两个小技巧：由于“类实例变量太多”或者“类内有太多代码”而导致出现重复代码时，说明这个类改拆分重构了。

建议：所谓的将大类拆解成小类，本质上在做的工作是分析工作，学习《软件方法》吧。

警惕：发现重复代码时。

六、长参数列表

问题：如果一个函数参数超过4个，那么可以考虑该函数是否可以优化。

建议：

1、将参数列表封装成对象，在支付常见的就是在proto文件中定义Message而不是平铺；

2、动静分离，原本应该属于静态结构的部分却以动态参数的方式传来传去，无形之中拉长了参数列表，举例说明如下：

// bad
void GetChapters(int book_id, HttpClient http_client) {    
 ……   
 httpClient.execute();  
}
// good
void GetChapters(int book_id) {    
 ……   
 HttpClient http_client = XXX;    
 httpClient.execute();  
}

 3、告别标记，不要用标记（布尔值、枚举值等形式很多）控制路径，在重构中这种手法叫做移除标记参数（Remove Flag Argument），举例如下：

// bad case
int EditChapter(int chapter_id, boolean apporved){ 
 if(apporved) {   
  ……  
 }else{   
  ……  
 }
}
//good
int EditChapter(int chapter_id);
int EditChapterWithApproval(int chapter_id);

警惕：当你调用一个函数记不住参数，要频繁的查看声明时就说明它该重构了。

七、滥用控制语句 7.1 嵌套的代码

问题：平铺直叙地写代码。

建议：以卫语句取代嵌套的条件表达式（Replace Nested Conditional with Guard Clauses），举例：

// bad case
int DistributeEpub(Epub epub) {  
 if (epub.IsValid()) {        
  boolean registered = RegisterIsbn(epub);        
  if (registered) {             
   ……    
  }  
 }
}
// good case
int DistributeEpub(Epub epub) {  
 if (!epub.IsValid()) {        
  return 0;
 } 
 boolean registered = RegisterIsbn(epub);     
 if (registered) { 
  ……  
 } 
}

警惕：else 也是一种坏味道，这是挑战很多程序员认知的。

7.2 重复的switch

// 计算用户购买书籍的价格
int GetBookPrice(User user, Book book) {  
 int price = book.GetPrice();  
 switch (user.GetLevel()) {    
  case UserLevel::SILVER:      
   return price * 0.9;    
  case UserLevel::GOLD:       
   return price * 0.8;  
 }
}
// 计算用户购买EPUB电子书籍的价格
int  GetEpubPrice(User user, Book book) {  
 intprice = epub.GetPrice();  
 switch (user.GetLevel()) {    
  case UserLevel::SILVER:      
   return price * 0.95;    
  case UserLevel::GOLD:       
  return price * 0.85;  
 }
}

问题：用户实际支付的价格会根据用户在系统中的用户级别有所差异，级别越高折扣就越高，最类似的部分就是switch，这也是一种坏味道：重复的 switch（Repeated Switch）。

优化：引入一个 UserLevel 基类，不同级别用户声明为UserLevel 的子类，将 switch 消除掉。

建议:以多态取代重复的条件表达式（Relace Conditional with Polymorphism）。

警惕：当出现了大量相似switch时。

7.3 总结

循环和选择语句，可能都是坏味道。

八、缺乏封装 8.1 过长的消息链

std::string name = book.GetAuthor().GetName();

问题：想写出上面这段代码，得先了解 Book 和 Author 这两个类的实现细节，也就是说我们必须得知道作者的姓名是存储在作品的作者字段里的。这时你就要注意了：当你必须得先了解一个类的细节才能写出代码时，这就说明这个封装是失败的。

优化：Book类封装GetAuthorName方法。

建议：隐藏委托关系（Hide Delegate），遵循迪米特法则。

警惕：大量的get方法

8.2 基本类型的偏执

std::string bank_type = Bank.GetBankType();

问题：在支付bank_type定义为字符串类型，实际上bank_type是可枚举的，比如“aaa”肯定不是一个合法的银行类型，但是由于定义为string类型，代码中并不能判断是否合法。

优化：实现时可以定义一个BankType对象，将校验逻辑放在构造函数中。

建议：如果很多重复的校验逻辑散落在各处，可以考虑以对象取代基本类型（Replace Primitive with Object）。

警惕：见到大量重复的“校验基本类型合法性”代码时。

九、可变的数据 9.1 满天飞的setter

void Approve(int book_id) { 
 ...  
 book.SetReviewState(ReviewStatus::APPROVED);  
 ...
}

问题：很多人在写代码时会利用 IDE生成 setter，setter 同 getter 一样，反映的都是对细节的暴露。

优化：Book类使用approve()方法修改状态字段，而不是直接使用set去 *** 作字段。

建议：相比于读数据修改是一个更危险的 *** 作。缺乏封装再加上不可控的变化，setter 几乎是排名第一的坏味道。

警惕：大量的set方法。

9.2 可变的数据

// 字符串成员方法，替换字符
void replace(char old_char, char new_char);

问题：直接修改原来的字符串，如果我们的数据压根不让修改，犯下各种低级错误的机会就进一步降低了。

优化：替换并不是在原有字符串上进行修改，而是产生了一个新的字符串

std::string replace(char old_char, char new_char);

建议：Martin Fowler 在《重构》第二版里新增了可变数据作为一种坏味道，如果需要更新，就产生一份新的数据副本，而旧有的数据保持不变。

警惕：定义可修改的全局变量也是很危险的行为。

十、变量声明和赋值 10.1 变量的初始化

EpubState state = null;
CreateEpubResponse response = CreateEpub(request);
if (response.GetCode() == 201) { 
 state  = EpubStatus::CREATED;
} else { 
 state = EpubStatus::TO_CREATE;
}

问题：先忽略else问题，这段代码还有一个问题：虽然 state 这个变量在声明的时候就赋上了一个 null 值，但实际上个值并没有起到任何作用，从语义上说第一行的变量初始化其实是没有用的，这是一次假的初始化。

优化：将state赋值逻辑封装为一个函数，EpubState state = XXX();

建议：变量初始化最好一次性完成，能使用const就使用const。

警惕：同理，map、list等类型先声明再添加数据也是坏味道。

十一、尾巴

其实我在学习课程时发现很多问题都是分析工作流没有做好，这里推荐学习潘加宇《软件方法》，分析做好了很多“代码的坏味道”自然而然也就没有了。