说说代码的坏味道

说说代码的坏味道

最近公司在要求学习极客时间的《代码之丑》，我花了大概一两天的时间看了一下，篇幅不长，很容易理解，基本看完了。我秉承着学习是一个输入的过程，但也不要忘了输出。

在这个专栏里，作者是列举了15项左右的代码坏味道清单。我不完全认同，有一些很难避免，也有一些我觉得问题不大。比如不要用else，尽量所有参数都用 final 修饰等等

下面，我简单整理了命名、大类、长参数、ifelse、封装，不一致、落后的代码风格等等几项。

命名

比如 processChapter（处理文章），你可能一看，发现没什么大问题。但是这种命名就太过于抽象，太泛，你完全不知道这个方法具体处理了什么时候。假如这个方法的主要处理逻辑是把一个 chapter 的翻译状态改成翻译中，然后你尝试把它改成changeChapterToTranslating()。是的，这个方法名称是比之前的好了，至少知道了它干了啥，但是这个方法名字暴露了实现细节。一个好的名字应该描述意图，而非细节。所以，可以尝试改成 startTranslation() 或许更好一点。

除此之外

• 不能用技术术语命名，因为接口时稳定的，而实现时易变的，比如不能用 bookList 这样的命名就不够好，而应该用books。
• 实际代码中，技术名称的出现，比如redis 代表的就是具体的技术实现，而应该用cache这样更广泛的词。

再简单说一个乱用英文命名问题。

比如 completedTranslate（完成翻译）。咋一看，这个方法名称没什么问题，甚至 completed 还用了完成时。其实这个方法不符合一般方法的命名规则。

命名规则：类名是一个名词，表示一个对象，而方法名则是一个动词，或者是动宾短语，表示一个动作.

所以，此时改成 completeTranslation 也许更好。

长函数

很多时候之所以出来长函数，是因为在 service 方法中平铺叙述的整个代码逻辑。比如业务代码和封装实体代码等不同层次的代码混在一起。

不知道你们有没有这个的感觉，反正当我在看一个函数时，我希望这个函数在一个屏幕可以看完。如果很长，需要多次滚动鼠标的话，可能会造成看了下面忘了上面的逻辑，可读性就不太好。

解决长函数的常用方法是提取函数。

顺便说一下，长函数往往还隐含着一个命名问题。命名很可能会冲突，可能就造成某一些的变量名字很长，增加了理解成本。

提取函数了的话，因为变量都是在这个短小的上下文里，也就不会产生那么多的命名冲突，变量名当然就可以写短一些。

大类

一个人理解的东西是有限的，没有人能同时面对所有细节。如果一个类里面的内容太多，它就会超过一个人的理解范畴，顾此失彼就在所难免了。

通常来说，很多类之所以巨大，大部分原因都是违反了单一职责原则。而想要破解“大类”的谜题，关键就是能够把不同的职责拆分开来

可能有人会说，拆分类的话会造成类太多的问题。是的，但我觉得没关系。

作为 Java 程序员，你不会去看所有 JDK 里的类，也不会看 Spring 所有的类。

一般的做法是理解主线，然后，根据需要了解相应的类，这是做事方法的问题。不能因为我们可能要面对所有代码，就一下子去吃透所有的代码，这是普通人做不到的。

所以，类的数量多少不是问题，通过怎样的方式，降低代码理解的难度才是我们要考虑的问题。

长参数

同样的，长参数也是一种坏味道。不知道你们有没有觉得，如果需要横着拉动鼠标才能看完整个参数列表，真的好烦。

有人可能会说，长参数的话，每个声明参数都换一行展示也还好吧。

我个人认为，这确实比长参数都在一行展示友好，但如果有代码洁癖的话，觉得这样不太好看，不够整洁。

通常情况下，解决长参数的常用方法是封装参数。可能有人又要说了，所有参数封装成对象的话，那调用该方法的地方就要写很多的setter 方法。这曾经也是我的疑问，我觉得这个问题可以在一定程度上通过 builder 构造器来解决。如果参数过多的话，确实也还会造成链条过长。但我觉得至少比在方法中的长函数要友好。

另外，如果参数中有标记参数的话，也可以通过拆分参数的方式减少参数。

if else

在极客专栏中，作者有一个极端的建议，代码中不要用else。我不敢苟同。我觉得一些简单的函数或者if else嵌套不是很深的话，if else 问题不大。

以我目前的开发经验的话，if else 大部分场景可以通过 不满足这个检查条件时，立刻从函数中返回来解决。另外有些场景也可以通过三运运算符，Stream，策略模式等来解决。

缺乏封装

看一段简单的代码

String name = book.getAuthor().getName();

这行代码看上去没啥大问题。但是如果是其他实体的话，对于一个不太熟悉业务的人来说就不太友好了。因为，当我获取这本书的名字的时候，我需要先获取这本书的作者，然后再获取作者的名称，这是有问题的。

当你必须得先了解一个类的细节，才能写出代码时，这只能说明一件事，这个封装是失败的。

其实可以通过把获取书的作者名称方法封装在 book 对象中。

变量声明和普通赋值相隔太远

List<Permission> permissions = new ArrayList<>();//声明
permissions.add(Permission.BOOK_READ);//一一赋值
permissions.add(Permission.BOOK_WRITE);
check.grantTo(Role.AUTHOR, permissions);

这段代码也有点坏味道，声明和赋值分离。这种情况有一个友好的写法。

一次完成声明和吃实话
List<Permission> permissions = ImmutableList.of(//Arrays.asList也可以
  Permission.BOOK_READ, 
  Permission.BOOK_WRITE
);
check.grantTo(Role.AUTHOR, permissions);

又比如

private static Map<Locale, String> CODE_MAPPING = new HashMap<>();//声明
...
//  中间一堆属性

// 赋值
static {
  CODE_MAPPING.put(LOCALE.ENGLISH, "EN");
  CODE_MAPPING.put(LOCALE.CHINESE, "CH");
}

这个例子就是变量声明和普通赋值相隔太远。增大了理解难度。可以尝试改成下面这样：

private static Map<Locale, String> CODE_MAPPING = ImmutableMap.of(
  LOCALE.ENGLISH, "EN",
  LOCALE.CHINESE, "CH"
);

不一致的代码

我们来看专栏的一个例子：这是一段在翻译引擎中创建作品的代码

public void createBook(final List<BookId> bookIds) throws IOException {
  // 根据bookIds获取book列表
  List<Book> books = bookService.getApprovedBook(bookIds)
  // 构建CreateBook参数
  CreateBookParameter parameter = toCreateBookParameter(books)
  // CreateBook参数 在转换成 HttpPost
  HttpPost post = createBookHttpRequest(parameter)
  httpClient.execute(post)
}

业务的话，简单来说，就是根据要处理的作品 ID 获取其中 book，然后，发送一个 HTTP 请求创建出这个作品。
这么短的一段代码有什么问题吗？问题就在于这段代码中的不一致。你可能会想：“不一致？不一致体现在哪里呢？”答案就是，这些代码不是一个层次的代码。

首先是获取审核通过的作品，这是一个业务动作

List<Book> books = bookService.getApprovedBook(bookIds)

接下来的三行其实是在做一件事，也就是发送创建作品的请求。具体到代码上

CreateBookParameter parameter = toCreateBookParameter(books)
HttpPost post = createBookHttpRequest(parameter)
httpClient.execute(post)

这三行代码分别是创建请求的参数，根据参数创建请求，最后，再把请求发送出去。这三行代码合起来完成了一个发送创建作品请求这么一件事，而这件事才是一个完整的业务动作。

所以，我说这个函数里的代码并不在一个层次上，有的是业务动作，有的是业务动作的细节。理解了这一点，我们就可以把这些业务细节的代码提取到一个函数里：

public void createBook(final List<BookId> bookIds) throws IOException {
  List<Book> books = bookService.getApprovedBook(bookIds)
  createRemoteBook(books)
}

private void createRemoteBook(List<Book> books) throws IOException {
  CreateBookParameter parameter = toCreateBookParameter(books)
  HttpPost post = createBookHttpRequest(parameter)
  httpClient.execute(post)
}

从结果上看，原来的函数 createBook 里面全都是业务动作，而提取出来的函数 createRemoteBook 则都是业务动作的细节，各自的语句都是在一个层次上了。

落后的代码风格

很多Java程序员编码习惯还停留 Java 8 之前写法。比如日期对象，对象的判空（Optional），循环处理，集合的过滤（Stream）操作等。其实这写在java8中都有比之前更加简洁的写法。还不清楚的可以先去学习一波…

另外提醒一点，不要在 lambda 中写过多的代码。lambda 中写出大片的代码，根本就是违反 lambda 设计初衷的。最好的 lambda 应该只有一行代码

如果 lambda 中的业务逻辑实现确实不是一行代码就可以的，那怎么办？提取函数到外面，然后 lambda 中直接调用。

最后，不断学习“新”的代码风格，不断改善自己的代码