10k

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

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

通过一段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的关注点。

结合上述点,可以看下之前的实现存在的问题:

  1. 他的实现只有一个类,比较简单,不太存在过度使用设计原则和模式,也没有违反基本的各项设计原则。
  2. 但是IDGenrator被设计成实现类而非接口,调用者直接依赖实现类,违反基于接口而非实现编程的思想。在当前情况下,问题不大,因为我们只有对后端请求,一种ID生成算法,直接改这个方法即可。但是如果项目中存在另一个ID生成算法,这个时候使用接口然后分别实现会好些。
  3. 静态方法generate不好测试,而且它本身也依赖一些运行环境(本机名)、时间函数、随机函数。
  4. 可读性不好,没注释,很多魔法数。

然后是业务实现上的问题:

  1. 首先是,这个随机是可能重复的,虽然概率比较小但是也是有可能出现的;另外,对于hostName为空的情况他并没有处理;而且对于异常他没有抛出,只打印了一条log记录。
  2. 日志方面打印得当简洁。只有一个generate接口,也不存在不易用的问题。generate中没有共享变量,所以代码安全。
  3. 性能方面,不依赖外部存储,打印频率也不高,足以应对目前的情况。但是ID每次生成都需要获取本机名,可以考虑优化;另外randoAscii生成的范围是0-122, 可用数字只是(0~9,a~z,A~Z),随机生成的字符中会存在一些不可用的,需要再次生成,这个地方也可优化。

还有一些其他的点,非共性问题:

  1. 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;
    }
}

先研究一下最后一个改动,实现类的命名

img

  • 第一种,如果我们要换一个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这个方法比较简单在提取之后,可以不测。
  • getLastSubstrSplittedByDotgenerateRandomAlphameric都改为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);

四个函数后两个比较复杂,所以重点需要测试。我们在之前的重构已经隔离了时间、随机等函数。当前我们只需要设计好测试用例并实现即可。

  1. protected String getLastSubstrSplittedByDot(String hostName);
    • hostName为null or ""
    • hostName为正常值(一个IP地址)
    • hostName是一个不valid 的ip地址(123.999.999.999)
    • hostName 的分隔符不是.(123#233#123#999)
  2. 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) {
    //...
  }
}
Thoughts? Leave a comment