设计模式之美学习-如何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代码

基础代码质量

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

review结果

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

业务代码质量

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

review结果

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

重构

1.提高代码的可读性

  1. hostName 变量不应该被重复使用，尤其当这两次使用时的含义还不同的时候
  2. 将获取 hostName 的代码抽离出来，定义为 getLastfieldOfHostName() 函数；
  3. 删除代码中的魔法数，比如，57、90、97、122；
  4. 将随机数生成的代码抽离出来，定义为 generateRandomAlphameric() 函数
  5. generate() 函数中的三个 if 逻辑重复了，且实现过于复杂，我们要对其进行简化；
  6. 对 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();
复制代码

 

提高代码的可测试性

  1. generate() 函数定义为静态函数，会影响使用该函数的代码的可测试性；
  2. 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) {
    //...
  }
}
复制代码

 

posted @   意犹未尽  阅读(317)  评论(0)  编辑  收藏  举报
编辑推荐：
· AI与.NET技术实操系列（二）：开始使用ML.NET
· 记一次.NET内存居高不下排查解决与启示
· 探究高空视频全景AR技术的实现原理
· 理解Rust引用及其生命周期标识（上）
· 浏览器原生「磁吸」效果！Anchor Positioning 锚点定位神器解析
阅读排行：
· 全程不用写代码，我用AI程序员写了一个飞机大战
· DeepSeek 开源周回顾「GitHub 热点速览」
· 记一次.NET内存居高不下排查解决与启示
· 物流快递公司核心技术能力-地址解析分单基础技术分享
· .NET 10首个预览版发布：重大改进与新特性概览！
历史上的今天：
2019-02-28 elasticsearch-搜索之中英文搜索(四)
点击右上角即可分享
微信分享提示
AI FOR CODE 大赛