设计模式之美学习-如何review代码发现代码质量问题(十四)

需求

需要开发一个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;
  }
}

整个 ID 由三部分组成。第一部分是本机名的最后一个字段。

第二部分是当前时间戳，精确到毫秒。

第三部分是 8 位的随机字符串，包含大小写字母和数字。尽管这样生成的 ID 并不是绝对唯一的，有重复的可能，但事实上重复的概率非常低。对于我们的日志追踪来说，极小概率的 ID 重复是完全可以接受的

运行结果

103-1577456311467-3nR3Do45
103-1577456311468-0wnuV5yw
103-1577456311468-sdrnkFxN
103-1577456311468-8lwk0BP0

如何review代码

基础代码质量

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

review结果

IdGenerator 的代码比较简单，只有一个类，所以，不涉及目录设置、模块划分、代码结构问题
也不违反基本的 SOLID、DRY（不要定义重复代码）、KISS(尽量保证简单)、YAGNI、LOD 等设计原则
它没有应用设计模式，所以也不存在不合理使用和过度设计的问题
IdGenerator 设计成了实现类而非接口，调用者直接依赖实现而非接口，违反基于接口而非实现编程的设计思想。后期需要修改就需要在原有代码修改，或则需要存在2种生成规则
并没有重复造轮子
并没有写单元测试
代码的可读性并不好。特别是随机字符串生成的那部分代码，一方面，代码完全没有注释，生成算法比较难读懂，另一方面，代码里有很多魔法数，严重影响代码的可读性。在重构的时候，我们需要重点提高这部分代码的可读性。

业务代码质量

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

review结果

id可能生成重复但是也满足我们现有需求
并没有处理host为空的情况
try catch了但是并没有往上抛或者打印error日志只打印了警告日志
并不涉及幂等和事物问题
没有涉及共享变量所以线程安全
每次生成 ID 都需要获取本机名，获取主机名会比较耗时，所以，这部分可以考虑优化一下。还有，randomAscii 的范围是 0～122，但可用数字仅包含三段子区间（0~9，a~z，A~Z），极端情况下会随机生成很多三段区间之外的无效数字，需要循环很多次才能生成随机字符串，所以随机字符串的生成算法也可以优化一下。

重构

1.提高代码的可读性

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

  public interface IdGenerator {
        String generate();
    }

    public class LogTraceIdGenerator implements IdGenerator {
        private static final Logger logger = LoggerFactory.getLogger(RandomIdGenerator.class);

        /**
         * 获得随id
         * @return
         */
        @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;
        }

        //获取hostName
        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;
        }
         //生成id
        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);
        }
    }

    //代码使用举例
    IdGenerator logTraceIdGenerator = new LogTraceIdGenerator();

提高代码的可测试性

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

    public class RandomIdGenerator implements IdGenerator {
        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;
        }

        /**
         * 获得hostname 将处理hostname剥离出来
         * 我们就只需要单独测试getLastSubstrSplittedByDot方法
         * @return
         */
        private String getLastfieldOfHostName() {
            String substrOfHostName = null;
            try {
                String hostName = InetAddress.getLocalHost().getHostName();
                substrOfHostName = getLastSubstrSplittedByDot(hostName);
            } catch (UnknownHostException e) {
                logger.warn("Failed to get the host name.", e);
            }
            return substrOfHostName;
        }

        /**
         * 测试就只需要测试这个方法就行了
         * @param hostName
         * @return
         */
        @VisibleForTesting
        protected String getLastSubstrSplittedByDot(String hostName) {
            String[] tokens = hostName.split("\.");
            String substrOfHostName = tokens[tokens.length - 1];
            return substrOfHostName;
        }

        /**
         * @VisibleForTesting 标识是为了测试将private改为protected
         * @param length
         * @return
         */
        @VisibleForTesting
        protected 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);
        }
    }

编写完善的单元测试

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或空字符串的情况
  //
  @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的情况
  //
  @Test
  public void testGenerateRandomAlphameric_lengthEqualsOrLessThanZero() {
    RandomIdGenerator idGenerator = new RandomIdGenerator();
    String actualRandomString = idGenerator.generateRandomAlphameric(0);
    Assert.assertEquals("", actualRandomString);

    actualRandomString = idGenerator.generateRandomAlphameric(-1);
    Assert.assertNull(actualRandomString);
  }
}

添加注释

/**
 * 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 IdGenerator {
  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) {
    //...
  }
}