IT博客汇 | 设计模式之美-课程笔记22-编程规范&代码质量实战（ID生成器）

设计模式之美-课程笔记22-编程规范&代码质量实战（ID生成器）

10k发表于 2023-07-23 00:00:00

两个内容：一是通过这个例子介绍如何发现问题，另外就是针对这个问题如何优化。

通过一段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) {
    //...
  }
}