设计模式学习笔记 – 规范与重构 – 7.实践：通过一段ID生成器代码，学习如何发现代码质量问题

前言

前面讲了重构相关的知识点。用一句话总结：重构就是发现代码质量问题，并且对其进行优化的过程。

今天借助一个 ID 生成器代码，给你展示以下重构的大致过程。

背景介绍

在软件开发中，ID 常用来表示一些业务信息的唯一标识，比如订单的单号、数据库中的唯一主键。

假设你正参与一个后端业务系统的开发，为了方便在请求出错时排查问题，在写代码的时候会在关键路径上打印日志。某个请求出错后，希望能搜索出这个请求对应的所有日志。而实际情况是，在日志文件中，不同请求的日志会交织在一起。如果没有东西来标识哪些日志属于同一个请求，我们就无法关联同一个请求的所有日志。

借助微服务调用链路追踪的实现思路，我们可以为每个请求分配一个唯一 ID，并保存在请求的上下文中（Context）。在 Java 语言中，我们可以将 ID 存储在 Servlet 线程的 ThreadLocal 中，或者利用 Sl4j 日志框架的 MDC（Mapped Diagnostic Contexts）来实现（实际上底层原理也是基于线程的 ThreadLocal）。每次打印时，我们就从请求上下文中取出请求 ID，来跟日志一起输出。这样，同一个请求的所有日志都包含同样的请求 ID 信息，我们就可以通过请求 ID 来搜索同一个请求的所有日志了。

通过一段 ID 生成器代码，学习如何发现代码质量问题

一份“能用”的代码

假设 leader 让小王负责这个 ID 生成器的开发。小王很快就完成了任务，将代码写了出来，具体如下所示：

public class IdGenerator {private static final Logger logger = LoggerFactory.getLogger(IdGenerator.class);public static String generate() {String id = "";try {String hostName = InetAddress.getLocalHost().getHostName();String[] tokens = hostName.split("\\.");if (tokens.length > 0) {hostName = tokens[tokens.length - 1];}char[] randomChars = new char[8];int count = 0;Random random = new Random();while (count < 8) {int randomAscii = random.nextInt(122);if (randomAscii >= 48 && randomAscii <= 57) {randomChars[count++] = (char) ('0' + (randomAscii - 48));} else if (randomAscii >= 65 && randomAscii <= 90) {randomChars[count++] = (char) ('A' + (randomAscii - 65));} else if (randomAscii >= 97 && randomAscii <= 122) {randomChars[count++] = (char) ('a' + (randomAscii - 97));}}id = String.format("%s-%s-%s", hostName,System.currentTimeMillis(), new String(randomChars));} catch (UnknownHostException e) {logger.error("failed to get the host name.", e);}return id;}}

上面的代码生成的 ID 示例如下所示。ID 由三部分组成。第一部分是本机名的最后一个字段。第二部分是当前时间戳，精确到毫秒。第三部分是8位随机字符串，包含大小写字母和数字。尽管这样生成的 ID 并不是绝对唯一的，但重复的概率非常低。对于我们的日志追踪来说，极小的概率的 ID 重复完全是可以接受的。

DESKTOP-Q24SEP3-1710335599009-Ql7pNHxTDESKTOP-Q24SEP3-1710335599014-nKfgvmSPDESKTOP-Q24SEP3-1710335599015-VkaJMwW9DESKTOP-Q24SEP3-1710335599015-6YChMRB6

不过，小王的这份代码只能算上 “能用”。 这段代码只有短短不到 40 行，里面却有很多值得优化的地方。

如何发现代码质量问题？

从大处着眼，我们可以参考之前讲过的代码评判标准，看这段代码是否可读、可扩展、可维护、灵活、简洁、可复用、可测试等等。

落实到细节，可以从以下这几个方面来审视代码。

• 目录设置是否合理、模块划分是否清晰、代码结构是否满足 “高内聚、松耦合”？
• 是否遵循经典的设计原则和设计思想（SOLID、DRY、KISS、YAGNI、LOD）?
• 设计模式是否使用得当？是否有过度设计？
• 代码是否易扩展？如果添加新功能，是否容易实现？
• 代码是否可以复用？是否可以复用已有的项目代码或类库？是否有重复造轮子？
• 代码是否容易测试？单元测试是否全面覆盖了各种正常和异常的情况？
• 代码是否易读？是否符合编码规范（如命名、注释、代码风格是否一致等）？

以上是通用的关注点，可以作用常规检查项，套用在任何代码的重构上。此外，我们还需要关注代码实现是否满足业务本身特有的需求。我罗列了一些比较共性的问题，如下所示：

• 代码是否实现了预期的业务需求？
• 逻辑是否正确？是否处理了各种异常情况？
• 日志打印是否得当？是否方便排查bug？
• 接口是否易用？是否支持幂等、事务等？
• 代码是否存在并发问题？是否线程安全？
• 性能是否有优化空间，比如，SQL、算法是否可以优化？
• 是否有安全漏洞？比如输入、输出校验是否全面？

现在，对照上面的检查，来看一下，小王编写的代码有哪些问题。

1. 首先，IdGenerator 代码比较简单，只有一个类，所以，不涉及目录设置、模块划分、代码结构问题，也不违反 SOLID、DRY、KISS、YAGNI、LOD 等设计原则。它没有应用设计模式，所以不存在不合理使用和过度设计问题。
2. 其次，IdGenerator 设计成了实现类而非接口，调用者直接依赖实现而非接口，违反基于接口而非编程原则的设计思想。不过，将 IdGenerator 设计成实现类，问题也不大。如果哪天 ID 生成算法改变了，只需要直接修改实现类的代码就可以。但是，如果项目中需要同时使用存在两种 ID 生成算法（也即要同时存在两个 IdGenerator 实现类），系统在使用的时候可以灵活选择生成算法，我们就需要将 IdGenerator 定义为接口，并且为不同的生产算法定义不同的实现类。
3. IdGenerator 的 genertae() 函数定义为静态函数，会影响使用该函数的代码的可测试性。同时，genertae() 函数的代码实现依赖于运行环境（本机名）、时间函数、随机函数，所以 genertae() 函数本身的可测试性也不好，需要做比较大的重构。此外，小王也没有编写单元测试代码，我们需要在重构时对其进行补充。
4. 最后，虽然 IdGenerator 只包含一个函数，且代码行数不多，但代码的可读性并不好。特别是随机字符串生成的那部分代码，一方面代码完全没有注释，生成算法比较难懂，另一方面代码里有很多魔法数，严重影响代码的可读性。在重构的时候，我们需要重点提高这部分代码的可读性。

刚刚我们参照跟业务无关的、通用的代码质量关注点，对小王的代码进行了评价。现在，我们在对照业务本身的功能和非功能需求，重新审视下小王的代码。

前面讲过，虽然小王的代码生成 ID 并非绝对唯一，但是对于追踪打印日志来说，是可以接受小概率 ID 冲突的，满足我们预期的业务需求。不过，获取 hostName 这部分代码逻辑有点问题，并未处理 “hostName 为空” 的情况。此外，尽管代码中针对获取不到本机名的情况做了异常处理，但是小王对异常的处理是在 IdGenerator 内部将其吞掉，然后打印一条报警日志，并没有继续往上抛出。这样的处理是否得当呢？你可以先考虑下，这部分内容在下一小节讲解。

小王代码的日志打印得当，日志描述能够准确反应问题，方便 debuf，并没有过多的冗余日志。IdGenerator 只暴露一个 genertae() 接口，接口的定义简单，不存在不易用的问题。genertae() 函数代码中没有涉及共享变量，所以代码线程安全，多线程环境下调用 genertae() 函数 不存在并发问题。

性能方面，ID 的生成不依赖外部存储，在内存中生成，并且日志的打印频率也不会很高，所以代码在性能方面足以应对目前的应用场景。不过，每次生存 ID 都要获取本机名，获取主机名会比较耗时，所以，这部分可以考虑优化下。还有 randomAscii 的范围是 0~122，但可用数据仅包含三个子段（0-9，a-z，A-Z），极端情况下会随机生成很多三段区间之外的无效数字，需要循环很多次才能生成随机字符串，所以随机字符串的生成算法也可以优化一下。

有一些代码质量问题不具有共性，需要你针对具体的业务、具体的代码去具体分析。小王的代码，你还能发现哪些问题？

在 genertae() 函数的 while 里面，三个 if 语句内部的代码非常相似，而且实现稍微有点过于复杂看，可以将这三个 if 合并在一起。

将 ID 生成器代码进行重构，从“能用”变为“好用”

制定重构计划

为方便对比，再把小王的代码贴一下。

public class IdGenerator {private static final Logger logger = LoggerFactory.getLogger(IdGenerator.class);public static String generate() {String id = "";try {String hostName = InetAddress.getLocalHost().getHostName();String[] tokens = hostName.split("\\.");if (tokens.length > 0) {hostName = tokens[tokens.length - 1];}char[] randomChars = new char[8];int count = 0;Random random = new Random();while (count < 8) {int randomAscii = random.nextInt(122);if (randomAscii >= 48 && randomAscii <= 57) {randomChars[count++] = (char) ('0' + (randomAscii - 48));} else if (randomAscii >= 65 && randomAscii <= 90) {randomChars[count++] = (char) ('A' + (randomAscii - 65));} else if (randomAscii >= 97 && randomAscii <= 122) {randomChars[count++] = (char) ('a' + (randomAscii - 97));}}id = String.format("%s-%s-%s", hostName,System.currentTimeMillis(), new String(randomChars));} catch (UnknownHostException e) {logger.error("failed to get the host name.", e);}return id;}}

前面，我们讲系统设计和实现的时候，我们讲到要循序渐进、小步快跑。重构代码的过程也应该遵循这样的思路。每次改动一点点，改好之后，再进行下一轮的优化，保证每次对代码的改动不会过大，能在很短的时间内完成。所以，将上一小节发现的代码质量问题，分成四次重构来完成，具体如下。

• 第一轮重构：提高代码的可读性。
• 第二轮重构：提高代码的可测试性。
• 第三轮重构：编写完善的单元测试。
• 第四轮重构：所有重构完成之后加注释。

第一轮重构：提高代码的可读性。

解决可读性问题，具体有下面几点：

• hostName 变量不应该被重复使用，尤其当两次使用时的含义还不同的时候。
• 获取 hostName 的代码抽离出来，定义为 getLastFieldOfHostName() 函数；
• 删除代码中的魔法数，比如，57、90、97、122；
• 将随机数生成的代码抽离出来，定义为 generateRandomAlphameric() 函数；
• genertae() 函数中的三个 if 逻辑重复了，且实现过于复杂，要将其进行简化；
• 对 IdGenerator 类重命名，并且抽象出接口。

先讨论下最后一个修改。实际上，对于 ID 生成器的代码，有下面三种类的命名方式。你觉得那种合适？

第一种命名方式，将接口命名为 IdGenerator，实现类命名为 LogTraceIdGenerator，这可能是很多人最先想到的命名方式。在命名时，我们要考虑这个两个类会如何使用、如何扩展。从使用和扩展的角度来分析，这样的命名就不合理了。

• 首先，如果我们扩展新的日志 ID 生成算法，因为原来的实现已经叫做 LogTraceIdGenerator 了，命名过于通用，那新的实现类就不好取名，无法取一个和 LogTraceIdGenerator 平行的名字了。
• 其次，假设我们没有日志 ID 的扩展需求，但要扩展其他业务的 ID 生成算法，比如针对用户的 UserIdGenerator、订单的 OrderIdGenerator，第一种命名方式是不是就合理了呢？答案也是否定的。基于接口而非实现编程，主要的目的就是为了方便后续灵活的替换实现类。而 LogTraceIdGenerator、UserIdGenerator、OrderIdGenerator 三个类从命名上来看，涉及的完全是不同的业务，不存在相互替换的场景。也就是说，我们不可能再日志的代码中，进行下面的替换。所以，让这三个类实现同一接口是没有意义的。

IdGenerator idGenerator = new LogTraceIdGenerator();// 替换为IdGenerator idGenerator = new UserIdGenerator();

第二种命名方式，也不合理。其中 LogTraceIdGenerator 是合理的，但是 HostNameMillisIdGenerator 暴露了太多实现细节，只要代码稍有改动，就可能需要改动命名，才能匹配实现。

第三种命名方式，是比较推荐的。在目前的 ID 生成器代码实现中，我们生成的 ID 是一个随机 ID，不是递增有序的，所以，命名成 RandomIdGenerator 是合理的，即便内部生成算法有所改动，只要生成的还是随机的 ID，就不需要改动命名。如果我们需要扩展新的 ID 生成算法，比如要实现一个递增有序的 ID 生成算法，那我们可以命名为 SequenceIdGenerator 。

实际上，更好的一种命名方式是，我们抽象出两个接口，一个的 IdGenerator，一个是 LogTraceIdGenerator， LogTraceIdGenerator 继承 IdGenerator。实现类实现接口 LogTraceIdGenerator，命名为 RandomIdGenerator 、SequenceIdGenerator。这样，实现类可以复用到多个业务模块中，比如前面提到的用户、订单。

根据上面的优化策略，我们对代码进行第一轮的重构。

public interface IdGenerator {String generate();}public interface LogTraceIdIdGenerator extends IdGenerator {}public class RandomIdGenerator implements LogTraceIdIdGenerator {private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class);@Overridepublic String generate() {String substrOfHostName = getLastFieldOfHostName();long currentTimeMillis = System.currentTimeMillis();String randomString = generateRandomAlphameric(8);String id = String.format("%s-%d-%s",substrOfHostName, currentTimeMillis, randomString);return id;}private String getLastFieldOfHostName() {String substrOfHostName = null;try {String hostName = InetAddress.getLocalHost().getHostName();String[] tokens = hostName.split("\\.");substrOfHostName = hostName = tokens[tokens.length - 1];} catch (UnknownHostException e) {logger.error("failed to get the host name.", e);}return substrOfHostName;}private String generateRandomAlphameric(int length) {char[] randomChars = new char[8];int count = 0;Random random = new Random();while (count < length) {int randomAscii = random.nextInt(122);boolean isDigit = randomAscii >= 48 && randomAscii <= 57;boolean isUpperCase = randomAscii >= 65 && randomAscii <= 90;boolean isLowerCase = randomAscii >= 97 && randomAscii <= 122;if (isDigit || isUpperCase || isLowerCase) {randomChars[count++] = (char) randomAscii;}}return new String(randomChars);}}// 代码使用举例LogTraceIdIdGenerator logTraceIdIdGenerator = new RandomIdGenerator();

第二轮重构：提高代码的可测试性

关于代码的可测试性的问题，主要包含下面两个方面：

• generate() 函数定义为静态函数，会影响使用该函数的代码的可测试性；
• generate() 函数的代码实现依赖于本机环境、时间函数、随机函数，所以 generate() 函数本身的可测试性也不好。

对于第一点，以及在第一轮重构中解决了。调用者可以通过依赖注入的方式，在外部创建好 RandomIdGenerator 对象后，注入到自己的代码中，从而解决静态函数调用影响代码可测试的问题。

对于第二点，我们需要在第一轮重构的基础上，再进行重构。重构之后的代码主要包括以下几个改动点。

• 从 getLastFieldOfHostName() 函数中，将逻辑比较复杂的那部分代码玻璃出来，定义为 getLastSubstrSplitByDot() 函数。因为 getLastFieldOfHostName() 本身依赖主机名，所以，剥离出主要代码之后，这个函数变得非常简单，可以不用测试。我们重点测试下 getLastFieldOfHostName()。
• generateRandomAlphameric() 和 getLastFieldOfHostName() 两个函数添加 Google Guava 的 annotation @VisibleForTesting。这个 annotation 没有任何实际作用，只起到标识的作用，告诉其他人说，这两个函数本该是 private 访问权限的，之所以提升访问权限到 protected，只是为了测试，只能用于单元测试中。

public class RandomIdGenerator implements LogTraceIdIdGenerator {private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class);@Overridepublic String generate() {String substrOfHostName = getLastFieldOfHostName();long currentTimeMillis = System.currentTimeMillis();String randomString = generateRandomAlphameric(8);String id = String.format("%s-%d-%s",substrOfHostName, currentTimeMillis, randomString);return id;}private String getLastFieldOfHostName() {String substrOfHostName = null;try {String hostName = InetAddress.getLocalHost().getHostName();substrOfHostName = getLastSubstrSplitByDot(hostName);} catch (UnknownHostException e) {logger.error("failed to get the host name.", e);}return substrOfHostName;}@VisibleForTestingprotected String getLastSubstrSplitByDot(String hostName) {String[] tokens = hostName.split("\\.");String substrOfHostName = tokens[tokens.length - 1];return substrOfHostName;}@VisibleForTestingprotected String generateRandomAlphameric(int length) {char[] randomChars = new char[8];int count = 0;Random random = new Random();while (count < length) {int randomAscii = random.nextInt(122);boolean isDigit = randomAscii >= 48 && randomAscii <= 57;boolean isUpperCase = randomAscii >= 65 && randomAscii <= 90;boolean isLowerCase = randomAscii >= 97 && randomAscii <= 122;if (isDigit || isUpperCase || isLowerCase) {randomChars[count++] = (char) randomAscii;}}return new String(randomChars);}}

打印日志的 Logger 对象被定义为 static final 的，并在类内部创建，这是否影响到代码的可测试性？是否应该将 Logger 对象通过依赖注入的方式注入到类中呢？

依赖注入之所以能提高代码的可测试性，主要是因为，通过这样的方式我们能轻松地用 mock 对象替代依赖的真实对象。那我们为什么要 mock 这个对象呢？这是因为，这个对象参与逻辑执行，但有不可控。对于 Logger 对象来说，我们只往里写入数据，并不读取数据，不参与业务逻辑的执行，不会影响代码逻辑的正确性，所以，我们没有必要 mcok Logger 对象。

此外，一些只是为了存储数据的对象，比如 String、Map、UserVo，我们也没必要通过依赖注入的方式来创建，直接在类中通过 new 创建就可以了。

第三轮重构：编写完善的单元测试

经过上面的重构之后，代码存在的比较明显的问题，基本上都已经解决了。现在为代码补全单元测试。RandomIdGenerator 类中有 4 个函数。

public String generate();private String getLastFieldOfHostName();@VisibleForTestingprotected String getLastSubstrSplitByDot(String hostName);@VisibleForTestingprotected String generateRandomAlphameric(int length)

先来看后两格函数。这两个函数包含的逻辑比较复杂，是我们测试的重点。而且，在上一步重构中，为了提高代码的可测试性，我们已经将不可控的组件（本机名、随机函数、时间函数）进行了隔离。所以，只需要设计完备的单元测试用例即可。具体的代码试下如下所示（使用了 JUint 测试框架）。

public class RandomIdGeneratorTest {@Testpublic void testGetLastSubstrSplitByDot() {RandomIdGenerator idGenerator = new RandomIdGenerator();String actualSubstr = idGenerator.getLastSubstrSplitByDot("field1.field2.field3");Assert.assertEquals("field3", actualSubstr);actualSubstr = idGenerator.getLastSubstrSplitByDot("field1");Assert.assertEquals("field1", actualSubstr);actualSubstr = idGenerator.getLastSubstrSplitByDot("field1#field2#field3");Assert.assertEquals("field1#field2#field3", actualSubstr);}// 此单元测试会失败，因为我们在代码中没有处理hostNam为null的或空字符的情况@Testpublic void testGetLastSubstrSplitByDot_nullOrEmpty() {RandomIdGenerator idGenerator = new RandomIdGenerator();String actualSubstr = idGenerator.getLastSubstrSplitByDot(null);Assert.assertNull(actualSubstr);actualSubstr = idGenerator.getLastSubstrSplitByDot("");Assert.assertEquals("", actualSubstr);}@Testpublic void testGenerateRandomAlphameric() {RandomIdGenerator idGenerator = new RandomIdGenerator();String actualRandomString = idGenerator.generateRandomAlphameric(6);Assert.assertNotNull(actualRandomString);Assert.assertEquals(6, actualRandomString.length());for (char c : actualRandomString.toCharArray()) {Assert.assertTrue(('0' <= c && c <= '9') || ('a' <= c || c <= 'z') || ('A' <= c || c <= 'Z'));}}// 此单元测试会失败，因为我们在代码中没有处理length<=0的情况@Testpublic void testGenerateRandomAlphameric_lengthEqualsOrLessThenZero() {RandomIdGenerator idGenerator = new RandomIdGenerator();String actualRandomString = idGenerator.generateRandomAlphameric(0);Assert.assertEquals("", actualRandomString);actualRandomString = idGenerator.generateRandomAlphameric(-1);Assert.assertNull(actualRandomString);}}

再来看下 generate() 函数。这个函数也是我们唯一一个暴露给外部使用的 public 函数。虽然逻辑比较简单，最好还是测试一下。但是，它依赖主机名、随机数、时间函数，我们该如何测试呢？需要 mock 这些函数的实现吗？

实际上要分情况下。写单元测试的时候，测试对象是函数定义的功能，而非具体的实现逻辑，函数的实现逻辑改变了之后，单元测试仍然可以工作。那 generate() 函数实现的功能是什么呢？这完全由代码编写者自己来定义。

比如，针对同一份 generate() 函数的代码实现，可以有 3 种不同的功能定义，对应三种不同的单元测试。

1. 如果我们把 generate() 函数的功能定义为： “生成一个随机唯一 ID”，那我们只要测试多次调用 generate() 函数生成的 ID 是否唯一即可。
2. 如果我们把 generate() 函数的功能定义为： “生成一个只包含数字、大小写字母和中划线的唯一 ID”，那我们不仅要测试 ID 的唯一性，还要测试生成的 ID 是否包含数字、大小写字母和中划线。
3. 如果我们把 generate() 函数的功能定义为： “生成唯一 ID，格式为 {主机名 substr}-{时间戳}-{8 位随机数}。在主机名获取失败时，返回：null-{时间戳}-{8 位随机数}”，那我们不仅要测试 ID 的唯一性，还要测试 ID 是否完全符合格式要求。

单元测试如何写，关键看你如何定义函数。针对 generate() 函数的前两种定义，我们不需要 mock 获取主机名，让其返回 null，测试代码运行是否符合预期。

最后，我们来看下 getLastFieldOfHostName() 函数。实际上，这个函数不容易测试，因为它调用了一个静态函数（InetAddress.getLocalHost().getHostName()），且这个静态函数依赖运行环境。但是，这个函数的实现非常简单，肉眼基本上可以排除明显的 bug，所以我们可以不为其编写单元测试代码。毕竟，我们写单元测试的目的是为了减少代码bug，而不是为了写单元测试而写单元测试。

如果你真的要对它测试，也有办法。

• 一种办法是使用更加高级的测试框架。比如 Power Mock，它可以 mock 静态函数。
• 另一种方式是将获取本机名的逻辑再封装为一个新的函数。

不过后遗症办法会造成代码过度零碎，也会稍微影响代码的可读性，这个需要你自己去权衡利弊来做选择。

第四轮重构：所有重构完成之后加注释

前面讲过，注释不能太多，也不能太少，主要添加在类和函数上。对于变量，好的命名可以替代注释，能清晰的表达函数。但对于类和函数来说，它们包含的逻辑往往比较复杂，单纯靠命名很难清晰地表明实现了什么功能，这个时候就需要通过注释来补充。

如何写注释，你可以参考《规范与重构 – 6.快速改善代码质量的20条编程规范》中注释的讲解。回顾下，注释主要是写清：做什么、为什么、怎么做，怎么用，对一些边界条件、特殊情况进行说明，以及对输入、输出、异常进行说明。

/** * Id Generator that is used to generate random IDs. * * 
 * The IDs generated by this class are not absolutely unique, * but the probability of duplication is very low. */public class RandomIdGenerator implements LogTraceIdIdGenerator {private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class);/** * Generate the random ID. The ID maybe duplicated only in extreme situation. * * @return a random ID */@Overridepublic String generate() {// ...}/** * Get the local hostname and * extract the last field of the name string splitted by delimiter '.'. * * @return the last field of hostname. Return null if hostname is not obtained. */private String getLastFieldOfHostName() {// ...}/** * Get the last field of {@hostName}splitted by delimiter '.' * * @param hostName should not be null * @return the last field of {@hostName}. Return empty string if {@hostName} is empty string. */@VisibleForTestingprotected String getLastSubstrSplitByDot(String hostName) {// ...}/** * Get random string which only contains digits, uppercase letters and lowercase letters. * * @param length should not be less then 0 * @return the random string. Returns empty string if {@length} is 0 */@VisibleForTestingprotected String generateRandomAlphameric(int length) {// ...}}