两个内容: 一是通过这个例子介绍如何发现问题,另外就是针对这个问题如何优化。
通过一段ID生成器代码,学习如何发现代码质量问题
ID生成器需求背景介绍
需求:为了方便在请求出错时排查问题,我们在编写代码的时候会在关键路径上打印日志。某个请求出错之后,我们希望能搜索出这个请求对应的所有日志,以此来查找问题的原因。而实际情况是,在日志文件中,不同请求的日志会交织在一起。如果没有东西来标识哪些日志属于同一个请求,我们就无法关联同一个请求的所有日志。
实现:我们可以给每个请求分配一个唯一 ID,并且保存在请求的上下文(Context)中,比如,处理请求的工作线程的局部变量中。在 Java 语言中,我们可以将 ID 存储在 Servlet 线程的 ThreadLocal 中,或者利用 Slf4j 日志框架的 MDC(Mapped Diagnostic Contexts)来实现(实际上底层原理也是基于线程的 ThreadLocal)。每次打印日志的时候,我们从请求上下文中取出请求 ID,跟日志一块输出。这样,同一个请求的所有日志都包含同样的请求 ID 信息,我们就可以通过请求 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)); count++; } else if (randomAscii >= 65 && randomAscii <= 90) { randomChars[count] = (char)('A' + (randomAscii - 65)); count++; } else if (randomAscii >= 97 && randomAscii <= 122) { randomChars[count] = (char)('a' + (randomAscii - 97)); count++; } } id = String.format("%s-%d-%s", hostName, System.currentTimeMillis(), new String(randomChars)); } catch (UnknownHostException e) { logger.warn("Failed to get the host name.", e); } return id; } }
我觉得可以拆成更小的函数,方便测试,
generate()
也更清晰;另外可能就是这个函数不够通用,他的前三位是基于IP地址,那我其他的服务其实是不能用这个方法的。(可以将IP当成参数传入,没提供的话可以使用其他方式生成);
代码块之间,不够清晰,至少要有些空行提升可读性;
只处理了一种异常;
最后这个函数生成的:
103-1577456311467-3nR3Do45
103-1577456311468-0wnuV5yw
103-1577456311468-sdrnkFxN
103-1577456311468-8lwk0BP0
如何发现代码质量问题?
可以从之前学过的代码质量评判标准:可读、可扩展、可维护、可测试、可复用、灵活、简洁等。
- 目录设置是否合理、模块划分是否清晰、代码结构是否满足”高内聚、松耦合“?
- 是否遵循设计原则和设计思想?(SOLID,DRY,KISS,YAGNI,LOD)
- 设计模式是否应用得当?是否有过度设计?
- 代码是否容易扩展?如果要添加新功能,是否容易实现?
- 代码是否可复用?是否在重复造轮子?
- 是否容易测试?单测是否全面覆盖正常和异常情况?
- 代码是否易读?是否符合编程规范?
还需要结合业务本身的功能和非功能需求:
- 这个代码是否实现了预期的业务需求?
- 逻辑是否正确,是否处理了各种异常?
- 日志打印是否得当,是否方便debug问题?
- 接口是否易用?是否支持幂等、事务?
- 代码是否存在并发问题?是否线程安全?
- 性能是否有优化空间,比如SQL、算法是否可以优化?
- 是否有安全问题?比如输入输出校验是否全面?
上面这些点感觉也算是妥妥的Code Review的关注点。
结合上述点,可以看下之前的实现存在的问题:
- 他的实现只有一个类,比较简单,不太存在过度使用设计原则和模式,也没有违反基本的各项设计原则。
- 但是IDGenrator被设计成实现类而非接口,调用者直接依赖实现类,违反基于接口而非实现编程的思想。在当前情况下,问题不大,因为我们只有对后端请求,一种ID生成算法,直接改这个方法即可。但是如果项目中存在另一个ID生成算法,这个时候使用接口然后分别实现会好些。
- 静态方法generate不好测试,而且它本身也依赖一些运行环境(本机名)、时间函数、随机函数。
- 可读性不好,没注释,很多魔法数。
然后是业务实现上的问题:
- 首先是,这个随机是可能重复的,虽然概率比较小但是也是有可能出现的;另外,对于hostName为空的情况他并没有处理;而且对于异常他没有抛出,只打印了一条log记录。
- 日志方面打印得当简洁。只有一个generate接口,也不存在不易用的问题。generate中没有共享变量,所以代码安全。
- 性能方面,不依赖外部存储,打印频率也不高,足以应对目前的情况。但是ID每次生成都需要获取本机名,可以考虑优化;另外randoAscii生成的范围是0-122, 可用数字只是(0~9,a~z,A~Z),随机生成的字符中会存在一些不可用的,需要再次生成,这个地方也可优化。
还有一些其他的点,非共性问题:
- while循环中的三个if很相似,可以合并;
手把手带你将ID生成器代码从“能用”重构为“好用”
重构要尽量循序渐进,不要追求一步到位。这样每次改动不会很大,也会比较短时间完成。
-
第一轮重构:提升代码的可读性
-
第二轮重构:提升代码的可测试性
-
第三轮重构:编写单测
-
第四轮重构:重构完成后添加注释
第一轮重构:提升代码的可读性
首先,我们要解决最明显、最急需改进的代码可读性问题。具体有下面几点:
- hostName 变量不应该被重复使用,尤其当这两次使用时的含义还不同的时候;
- 将获取 hostName 的代码抽离出来,定义为 getLastfieldOfHostName() 函数;
- 删除代码中的魔法数,比如,57、90、97、122;
- 将随机数生成的代码抽离出来,定义为 generateRandomAlphameric() 函数;
- generate() 函数中的三个 if 逻辑重复了,且实现过于复杂,我们要对其进行简化;
- 对 IdGenerator 类重命名,并且抽象出对应的接口。
My change
package IDGenerator; import java.net.UnknownHostException; /** * Created by szhang on 2023/7/23 */ public interface IdGenerator { String generate() throws UnknownHostException; } package IDGenerator; import com.sun.org.slf4j.internal.Logger; import com.sun.org.slf4j.internal.LoggerFactory; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.Objects; import java.util.Random; /** * Created by szhang on 2023/7/23 */ public class IdGeneratorImpl implements IdGenerator { private static final Logger logger = LoggerFactory.getLogger(IdGeneratorV0Impl.class); private static final int RANDOM_CHAR_LENGTH = 8; @Override public String generate() throws UnknownHostException { String id; try { String hostStr = getHostName(); if (Objects.isNull(hostStr)) { throw new UnknownHostException(); } id = String.format("%s-%d-%s", hostStr, System.currentTimeMillis(), generateAlphabaticRandomChars()); } catch (UnknownHostException e) { logger.warn("Failed to get the host name.", e); throw e; } return id; } private String generateAlphabaticRandomChars() { StringBuilder sb = new StringBuilder(); Random random = new Random(); char[] availableChars = new char[]{'0', '1', '2', 'a', 'b', 'A', 'B'}; // 0-9, a-z, A-Z; for (int i = 0; i < RANDOM_CHAR_LENGTH; i++) { int randomAscii = random.nextInt(availableChars.length); sb.append(availableChars[randomAscii]); } return sb.toString(); } private String getHostName() throws UnknownHostException { String hostName = InetAddress.getLocalHost().getHostName(); String[] tokens = hostName.split("\\."); if (tokens.length > 0) { return tokens[tokens.length - 1]; } return null; } }
先研究一下最后一个改动,实现类的命名
- 第一种,如果我们要换一个ID生成算法,第一个的实现类的名字已经很通用了,比较难再取一个类似的平行名字。如果我们不改算法,我们拓展其他业务(UserIdGenerator),第一种也不合理。因为基于接口而非实现编程,主要目的就是后续可以灵活的替换,不同业务是无法替换的,是不同的应用场景:
IdGenearator idGenerator = new LogTraceIdGenerator(); 替换为: IdGenearator idGenerator = new UserIdGenerator();
让他们实现相同的接口,是没有大意义的。
- 第二种接口命名合理,但是实现类命名暴露太多细节(强绑定实现),代码一改变就可能也需要改命名,所以也不太好。
- 推荐第三种。
- 更好的一种方式是,抽象出两个接口,一个是IdGenerator,一个是LogTraceIdGenerator, LogTraceIdGenerator继承IdGenerator,实现类实现接口LogTraceIdGenerator,命名为RandomIdGenerator、SequenceIdGenerator 等。这样,实现类可以复用到多个业务模块中,比如前面提到的用户、订单。
总体的重构如下:
public interface IdGenerator { String generate(); } public interface LogTraceIdGenerator extends IdGenerator { } public class RandomIdGenerator implements LogTraceIdGenerator { private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class); @Override public 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 = tokens[tokens.length - 1]; return substrOfHostName; } catch (UnknownHostException e) { logger.warn("Failed to get the host name.", e); } return substrOfHostName; } private String generateRandomAlphameric(int length) { char[] randomChars = new char[length]; int count = 0; Random random = new Random(); while (count < length) { int maxAscii = 'z'; int randomAscii = random.nextInt(maxAscii); boolean isDigit= randomAscii >= '0' && randomAscii <= '9'; boolean isUppercase= randomAscii >= 'A' && randomAscii <= 'Z'; boolean isLowercase= randomAscii >= 'a' && randomAscii <= 'z'; if (isDigit|| isUppercase || isLowercase) { randomChars[count] = (char) (randomAscii); ++count; } } return new String(randomChars); } } //代码使用举例 LogTraceIdGenerator logTraceIdGenerator = new RandomIdGenerator();
第二轮重构:提高代码的可测试性
可测试性在之前的代码中主要体现为两问题:
generate
()是静态方法generate()
依赖时间函数、随机函数等
第一点已经在第一轮重构中解决。重新定义为普通函数。第二点还是需要对代码作进一步重构。
- 将
getLastfieldOfHostName
中除了获取hostName
的部分再次提取出来一个新函数getLastSubstrSplittedByDot()
,重点测试这个即可,getLastfieldOfHostName
这个方法比较简单在提取之后,可以不测。 - 将
getLastSubstrSplittedByDot
和generateRandomAlphameric
都改为protected
方法并且注解上Google Guava 的 annotation@VisibleForTesting
。这个 annotation 没有任何实际的作用,只起到标识的作用,告诉其他人说,这两个函数本该是private
访问权限的,之所以提升访问权限到protected
,只是为了测试,只能用于单元测试中。``
打印日志的 Logger 对象被定义为 static final 的,并且在类内部创建,这是否影响到代码的可测试性?是否应该将 Logger 对象通过依赖注入的方式注入到类中呢?
不需要,依赖注入提高代码可测性是因为他可以让我们mock一些对象,这些对象本身在原来的执行过程中相对复杂,通过mock我们搞一些简单的虚假的东西(非测试关注点),让测试代码变得简单。而这个地方Logger我们只是往里面写log,他也不会影响业务逻辑和代码执行正确性,没必要mock。
第三轮重构:编写完善的单测
public String generate(); private String getLastfieldOfHostName(); @VisibleForTesting protected String getLastSubstrSplittedByDot(String hostName); @VisibleForTesting protected String generateRandomAlphameric(int length);
四个函数后两个比较复杂,所以重点需要测试。我们在之前的重构已经隔离了时间、随机等函数。当前我们只需要设计好测试用例并实现即可。
protected String getLastSubstrSplittedByDot(String hostName);
- hostName为null or ""
- hostName为正常值(一个IP地址)
- hostName是一个不valid 的ip地址(123.999.999.999)
- hostName 的分隔符不是
.
(123#233#123#999)
protected String generateRandomAlphameric(int length);
- Length 是负数
- length是0
- length 是正整数
public class RandomIdGeneratorTest { @Test public void testGetLastSubstrSplittedByDot() { RandomIdGenerator idGenerator = new RandomIdGenerator(); String actualSubstr = idGenerator.getLastSubstrSplittedByDot("field1.field2.field3"); Assert.assertEquals("field3", actualSubstr); actualSubstr = idGenerator.getLastSubstrSplittedByDot("field1"); Assert.assertEquals("field1", actualSubstr); actualSubstr = idGenerator.getLastSubstrSplittedByDot("field1#field2#field3"); Assert.assertEquals("field1#field2#field3", actualSubstr); } // 此单元测试会失败,因为我们在代码中没有处理hostName为null或空字符串的情况 // 这部分优化留在第36、37节课中讲解 @Test public void testGetLastSubstrSplittedByDot_nullOrEmpty() { RandomIdGenerator idGenerator = new RandomIdGenerator(); String actualSubstr = idGenerator.getLastSubstrSplittedByDot(null); Assert.assertNull(actualSubstr); actualSubstr = idGenerator.getLastSubstrSplittedByDot(""); Assert.assertEquals("", actualSubstr); } @Test public 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的情况 // 这部分优化留在第36、37节课中讲解 @Test public void testGenerateRandomAlphameric_lengthEqualsOrLessThanZero() { RandomIdGenerator idGenerator = new RandomIdGenerator(); String actualRandomString = idGenerator.generateRandomAlphameric(0); Assert.assertEquals("", actualRandomString); actualRandomString = idGenerator.generateRandomAlphameric(-1); Assert.assertNull(actualRandomString); } }
对于generate函数,也可以测试一下,单测测试的是功能,不是具体的实现逻辑(没意义)。针对generate的代码实现,可以有三种不同的代码定义:
- ”生成一个唯一的随机ID“:那我们要测试的是多次调用后结果的唯一性;
- ”生成一个只包含数字、大小写字母和中划线的唯一ID“,我们既要测试唯一性,还要校验结果字符中只包含这三种字符;
- ”生成唯一ID,格式为:「主机名」-「时间戳」-「8位随机数」,主机名获取失败,返回「null」-「时间戳」-「8位随机数」。那么除了第二点中的校验点,我们还需要看它的格式。
单测如何写,关键是看coder如何定义函数。
getLastfieldOfHostName()的单测不好写,调用了时间函数,但是我们可以肉眼看排除bug,不一定需要单测。毕竟单测的目的是减少bug而非一定要写单测。一定要写的话有一些高级的框架方法也可以实现,或者是封装这些个函数,不过这个会导致代码零碎,需要权衡。
第四轮重构:添加注释
写清楚:做什么、为什么、怎么做、怎么用即可,对于一些特殊的边界、情况,或者是输入输出也可以说明。
/** * Id Generator that is used to generate random IDs. * * <p> * The IDs generated by this class are not absolutely unique, * but the probability of duplication is very low. */ public class RandomIdGenerator implements LogTraceIdGenerator { private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class); /** * Generate the random ID. The IDs may be duplicated only in extreme situation. * * @return an random ID */ @Override public String generate() { //... } /** * Get the local hostname and * extract the last field of the name string splitted by delimiter '.'. * * @return the last field of hostname. Returns null if hostname is not obtained. */ private String getLastfieldOfHostName() { //... } /** * Get the last field of {@hostName} splitted by delemiter '.'. * * @param hostName should not be null * @return the last field of {@hostName}. Returns empty string if {@hostName} is empty string. */ @VisibleForTesting protected String getLastSubstrSplittedByDot(String hostName) { //... } /** * Generate random string which * only contains digits, uppercase letters and lowercase letters. * * @param length should not be less than 0 * @return the random string. Returns empty string if {@length} is 0 */ @VisibleForTesting protected String generateRandomAlphameric(int length) { //... } }