你好,我是郑晔。
在这个专栏刚开始的时候,我给你留了一个课前作业,实现一个待办事项管理的软件。许多同学都利用自己的业余时间完成了这个作业,感谢大家的付出!
学习代码的坏味道,听别人讲是一种方式,但这种方式总会让人有一种隔岸观火的感觉,虽然知道有问题,但感觉并不深刻。最直接受益的方式就是自己写了代码,然后,让别人来点评。其实,这就是某种形式的代码评审。
所以,这一讲,我们就来做一次“代码评审”,直接来看看代码中存在的问题。题目背景我就不再做过多的介绍了,如果没有来得及完成作业的同学,可以先到“[课前作业区]”回顾一下题目。
既然是指出问题,得罪大家可能就在所难免了,希望你不要介意,毕竟能够发现自己的问题是精进的第一步。好,我们开始!
Item itemNew = new Item(item.getName());
itemNew.setUserIndex(userIndex);
itemNew.setIndex(initUserIndex);
我们的业务需求是添加 TODO 项,这段代码就是在这个过程中创建一个新的 TODO 项对象。那这段代码有什么问题?一方面,这里有 setter,另一方面,这里的 setter 只在初始化的过程中用到。显然,我们可以用一个更完整的构造函数替换掉它。
其实,从这段代码出发,我们还能看到一些小问题,比如,这里创建 TODO 项设置了两个字段,一个是 userIndex,一个是 index。index 可以理解,表示这个 TODO 项的索引,但 userIndex 是什么呢?你需要仔细阅读代码才能发现,它其实是一个用户的标识,表示这个索引项是由某个用户创建的。既然是用户标识,按照通常的做法它可以叫 userId,这就降低了理解的难度。
这段代码所在类的声明也是一个让人出戏的地方:
public class ProcessTxtServiceImpl implements ProcessItemservice
这个类实现了一个接口 ProcessItemservice,显然,这里的拼写是有问题的,它应该是 ProcessItemService,另外,它的名字叫做“处理(TODO)项的服务”,一方面,在一个服务名字上用了处理这个动词,另一方面,“处理”这个名字也是特别泛化的一个名字。如果是我来声明这个接口,它可能就叫 ItemService。
所以,你可以看到,仅仅是一个接口的命名,就有这么多的问题。
我们再来看这个类的命名 ProcessTxtServiceImpl,这个名字里有一个 Txt 是容易让人困惑的,一般来说,如果不是特别的原因,尽量不要用缩写。
我初看到这个名字时,着实想了半天它表示什么含义,一开始我以为是表示事务(Transaction),常有人把事务缩写成 Tx,如果它的含义是表示事务,那么这里就是一个拼写错误了。后来,我才想明白,这里的 Txt 表示的是文本(Text),仅仅省了一个字母,却造成了理解上更大的障碍,实在有些得不偿失。
如果 Txt 表示的是文本,这里就暴露出另外一个问题。这里为什么要有一个文本呢?其实是对应着另外一个数据库存储的实现,这是第四阶段的要求。
文本和数据库的差别到底是体现在哪里呢?体现在存储上。而在这段代码中,差别从服务层面就开始了,换言之,按照这段代码的逻辑,实现数据库存储,就需要把整个的业务逻辑重新写一遍。显然,这种做法是从结构上来看是有问题的,会造成大量的重复代码。
理解了文本和数据库只差别在存储这件事,我们再回过头来看这个类的声明。
public class ProcessTxtServiceImpl implements ProcessItemservice
这个为数据库预留的实现根本就是不需要的,只有一个 ItemService 的实现就够了,换言之,也就没有必要声明出一个接口,这里的类层次这么复杂,根本就是没有必要的。
这里我再补充一个点,很多 Java 程序员给类命名有个不好的习惯,用“I” 打头给接口命名,用“Impl”给实现类结尾,这其实是早期的一种编程习惯,准确地说,这就是没有想好命名的偷懒方式。其实,它也是我们讲到的“[用技术术语命名]”的一种具体体现方式。后来的代码基本上就不这么做了,因为我们可以找到更准确的描述。但很多人的编程习惯却留在了早期,所以,这也算是一种遗毒的吧。
接下来,我们再来看一个很多人代码中都存在的问题。
下面是来自刘大明同学的一段代码,这是一个用以存放用户信息的类。单看这段代码本身,其实写得还是非常不错的,代码本身并不长,而且考虑了很多的细节。我们暂且忽略其它的细节,我注意到这段代码的主要原因是因为它用到了 static:
public class UserContext {
private static ThreadLocal<Integer> USERID = new ThreadLocal();
private UserContext() {
throw new UnsupportedOperationException();
}
public static String getUserID() {
return String.valueOf(USERID.get());
}
public static void setUserID(Integer userID) {
USERID.set(userID);
}
}
在《10x 程序员工作法》讲到[测试驱动开发]时,我曾经讲了 static 函数的问题,简单总结一下就是:
从本质上说,static 函数是一种全局函数,static 变量是一种全局变量,全局的函数和变量是我们尽量规避的;
一个函数调用了 static 函数不好测试;
除了写程序库,日常开发尽可能不用 static 函数。
那怎么消除 static 函数呢?消除 static 函数,最简单的做法就是用普通的函数调用替换掉 static 函数,也就是把这里的 static 都去掉。涉及到相应的字段,也要去掉 static。这种做法没有问题,但通常这种做法太粗暴了。这里我们尝试着用重构的方式一步一步地把它替换掉。
首先,我要去掉这里的构造函数,因为这里的构造函数是私有的,无法调用,而我们要用普通的函数,自然就需要构造出一个对象来。
public class UserContext {
private static ThreadLocal<Integer> USERID = new ThreadLocal();
public static String getUserID() {
return String.valueOf(USERID.get());
}
public static void setUserID(Integer userID) {
USERID.set(userID);
}
}
然后,我们需要找到对应的调用点,这里就以其中的一个为例,下面就是在退出登录的地方调用了这里的 static 函数:
public class UserAccounts {
...
public void loginOut() {
UserContext.setUserID(null);
}
}
我们可以把它改成对象的调用:
public class UserAccounts {
...
public void loginOut() {
new UserContext().setUserID(null);
}
}
这样,我们就有了一个对象,因为原来的函数是 static 函数,所以,这里的调用,本质上还是原来的函数,所以不会有影响。
然后,我们把这个创建出的对象变成这个类的字段,如果你使用的是支持重构功能的 IDE,这就是一个快捷键的操作(引入字段,Introduce Field):
public class UserAccounts {
...
private UserContext context = new UserContext();
public void loginOut() {
context.setUserID(null);
}
}
如果在一个类有多个调用点,不妨都改成这个新字段的函数调用,正如我们前面所说,目前还是一个 static 函数,无论从哪个对象调用,调用的都是同一个函数。
通常来说,这个 static 函数应该不只是在一个类中使用,所以,它应该是在多个类中间共享的,为了保证多个类中间使用同一个 UserContext 对象,UserContext 对象的初始化就不能在这个类进行,而要在同一个地方初始化,所以,我们这里可以把 UserContext 对象作为构造函数的参数传进来:
public class UserAccounts {
...
private UserContext context;
public UserAccounts(..., final UserContext context) {
...
this.context = context;
}
public void loginOut() {
context.setUserID(null);
}
}
有了这个基础,我们再在 UserAccounts 这个对象初始化的时候,把这个 UserContext 对象传进来:
new UserAccounts(..., new UserContext());
如此一来,UserContext 这个对象的初始化就放到对象组装的过程中了,这就可以在多个不同的对象组件中共享这个对象了。如此往复,将所有的调用点都这么修改,我们就消除了对于 static 函数的依赖。现在,我们可以动手消除 static 了:
public class UserContext {
private ThreadLocal<Integer> USERID = new ThreadLocal();
public String getUserID() {
return String.valueOf(USERID.get());
}
public void setUserID(Integer userID) {
USERID.set(userID);
}
}
消除 static 函数本身并不难,这里我是借着这个简单的例子,给你演示一下,如何一步一步地进行重构。可能这比很多人以为的大刀阔斧地修改代码来得要琐碎得多,但只有这样一点一点调整,代码足够安全,每一步都是能够停下来的。
无论如何,请别忘了,真正能给予我们修改有效性回答的是,单元测试。
估计很多人看到这里就会说,如果 static 都成了坏味道,那 Singleton 模式该怎么办呢?答案就是尽可能不用 Singleton 模式。我在《软件设计之美》中讲[可测试性]和[设计模式]时,都说到过 Singleton 模式,简单地说,系统里只有一个实例和限制系统里只能构建出一个实例,这是两件事,而且,如果一个函数牵扯到 Singleton 类也不好测试。
在一些同学的代码中,我也看到的 Singleton 模式的使用,处理手法其实与这里消除 static 函数是类似的,只不过,Singleton 稍微好一点的是,它的函数和字段本身都已经是普通的类成员了,我们只需要把那个限制实例唯一的 static 函数和字段消除就可以了。
说了半天的代码问题,我还想对很多人普遍忽略的小问题说上几句,这就是文档,对应到各位的代码库中,主要就是 README。
一个开源项目的好坏与否,同它的文档质量是强相关的。我知道,作为程序员,大家的普遍兴趣都是写代码,所以,文档就常常被忽略了。
如果我不了解这个项目的背景,很多人的 README 给我提供的信息量是非常有限的。
大家的 README 普遍存在的问题有两种,一种是信息量太少,比如,只写了如何构建一个项目,另一种是把 README 当成 blog,在里面写了自己的心得体会。无论是哪种,信息的有效性都很差。
README 文件是一个项目的门面,它应该给我们提供关于这个项目的背景信息,比如,这个项目是做什么的、当前的状态、如何入手等等。你可以找一些经典的开源项目,去看看好的 README 是怎么写的。好的程序员要学会表达,不仅仅会用代码表达,也要会用文字表达。
好,这就是大家作业中的所有问题了吗?当然不是,代码中存在的问题还很多。不过,你不用担心,即便这个专栏的正式更新结束了,我也会考虑以加餐的形式,继续我们这个云端的代码评审环节。所以,之前没来得及写代码的同学依然可以继续写,说不定下次就会谈到你的代码。
今天我们点评了大家代码中存在的一些问题,除了之前在专栏中讲到过的坏味道,今天我们还讲到了一些一眼就可以看出问题的坏味道:
使用缩写;
用 I 表示接口,用 Impl 表示实现;
使用 static 函数或变量;
使用 singleton 模式;
写得潦草的 README。
你在写代码时也要注意这些问题。
我还借着 static 函数的调整过程,给你演示了如何一步一步地重构代码,保证代码的安全。希望你能够理解,重构不是大开大合的过程,而就是这样细小而微的操作。
如果今天的内容你只能记住一件事,那请记住:尽量不使用 static。
我们今天谈到了文档,你平时写文档吗?或者,你平时阅读项目文档,发现什么值得改善的地方吗?欢迎在留言区分享你的经验。