如何code review代碼？

代碼code review 從大處著眼可以從可讀性、可維護(hù)性、可擴(kuò)展、可復(fù)用、可測(cè)試等方面來(lái)衡量；落實(shí)到具體細(xì)節(jié)，可以從非功能性和功能性兩個(gè)方面來(lái)進(jìn)行衡量。

非功能性

1. 目錄設(shè)置是否合理、模塊劃分是否清晰、代碼結(jié)構(gòu)是否滿足“高內(nèi)聚，低耦合”特性；
2. 是否遵循經(jīng)典設(shè)計(jì)原則與設(shè)計(jì)思想，如：SOLID、DRY、KISS、YAGNI和LOD等；
3. 設(shè)計(jì)模式是否應(yīng)用合理，是否過度設(shè)計(jì)；
4. 代碼是否易擴(kuò)展，是否滿足開閉原則；
5. 代碼是否可復(fù)用，是否可以復(fù)用已有代碼或類庫(kù)，是否存在重復(fù)造輪子；
6. 代碼是否易測(cè)試，UT對(duì)正常和異常邊界情況是否覆蓋全面；
7. 代碼是否可讀，是否滿足編碼規(guī)范，如：命名是否準(zhǔn)確達(dá)意、注釋是否恰當(dāng)、代碼風(fēng)格是否一致、編程是否存在多層嵌套，復(fù)雜邏輯或多個(gè)入?yún)⑶闆r是否進(jìn)行拆分或封裝等。

功能性

1. 代碼設(shè)計(jì)是否符合功能預(yù)期；
2. 邏輯是否正確，異常邊界情況是否處置合理；
3. 日志打印是否得當(dāng)，是否方便排查問題？
4. 接口是否易用，是否支持CUD場(chǎng)景事務(wù)和冪等操作；
5. 并發(fā)場(chǎng)景下代碼是否線程安全，是否存在共享變量；
6. 性能是否有優(yōu)化空間，如：SQL、算法是否可以繼續(xù)優(yōu)化；
7. 是否存在安全漏洞，輸入、輸出校驗(yàn)是否全面合理；

案例

需求

為了方便排查問題，請(qǐng)?jiān)O(shè)計(jì)一個(gè)ID 生成器，每次請(qǐng)求會(huì)將生成的ID 保存在 Servlet 線程的 ThreadLocal 中，每次打印日志的時(shí)候，我們從請(qǐng)求上下文中取出請(qǐng)求 ID，跟日志一塊輸出。這樣，同一個(gè)請(qǐng)求的所有日志都包含同樣的請(qǐng)求 ID 信息，我們就可以通過請(qǐng)求 ID 來(lái)搜索同一個(gè)請(qǐng)求的所有日志了。

實(shí)現(xiàn)

@Slf4j
public class IdGenerator {
    /**
     * 獲取請(qǐng)求ID
     *
     * @return
     */
    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){
            log.warn("Failed to get the host name.",e);
        }
        return id;
    }
}

優(yōu)化

非功能分析

功能分析

異常處理

函數(shù)出錯(cuò)返回?cái)?shù)據(jù)類型，我總結(jié)了 4 種情況分別是：錯(cuò)誤碼、NULL 值、空對(duì)象、異常對(duì)象。

錯(cuò)誤碼

在沒有異常語(yǔ)法機(jī)制的語(yǔ)言中，常用錯(cuò)誤碼來(lái)處理錯(cuò)誤，比如：code >= 0 表示接口調(diào)用成功；code < 0表示接口調(diào)用失??；同時(shí)code值也可以賦予特殊意義。

NULL值

在多數(shù)編程語(yǔ)言中，我們用 NULL 來(lái)表示“不存在”這種語(yǔ)義。對(duì)于查找函數(shù)來(lái)說(shuō)，數(shù)據(jù)不存在并非一種異常情況，是一種正常行為，所以返回表示不存在語(yǔ)義的 NULL 值比返回異常更加合理。

空對(duì)象

返回 NULL 值有各種弊端，對(duì)此有一個(gè)比較經(jīng)典的應(yīng)對(duì)策略，那就是應(yīng)用空對(duì)象設(shè)計(jì)模式。當(dāng)函數(shù)返回的數(shù)據(jù)是字符串類型或者集合類型的時(shí)候，我們可以用空字符串或空集合替代 NULL 值，來(lái)表示不存在的情況。這樣，我們?cè)谑褂煤瘮?shù)的時(shí)候，就可以不用做 NULL 值判斷。

異常對(duì)象

對(duì)于接口拋出的異常，我們有三種處理方法：直接吞掉、直接往上拋出、包裹成新的異常拋出。

適用場(chǎng)景

• 直接吞掉：如果 被調(diào)用方拋出的異常是可以恢復(fù)，的調(diào)用方并不關(guān)心此異常，我們完全可以在 調(diào)用方中拋出的異常吞掉；
• 直接往上拋出：如果被調(diào)用方拋出的異常對(duì)調(diào)用方來(lái)說(shuō)，也是可以理解的、關(guān)心的 ，并且在業(yè)務(wù)概念上有一定的相關(guān)性，我們可以選擇直接將 被調(diào)用方拋出的異常 re-throw；
• 包裹成新的異常拋出：如果被調(diào)用方拋出的異常太底層，對(duì)調(diào)用方來(lái)說(shuō)，缺乏背景去理解、且業(yè)務(wù)概念上無(wú)關(guān)，我們可以將它重新包裝成調(diào)用方可以理解的新異常，然后 re-throw。

優(yōu)化計(jì)劃

重構(gòu)代碼的過程需要遵循循序漸進(jìn)，小步快跑思路。每次改動(dòng)一點(diǎn)點(diǎn)，測(cè)試通過之后再進(jìn)行下一部分。針對(duì)本次重構(gòu)計(jì)劃可以分成四部分進(jìn)行，具體如下：

• 第一輪：提高可讀性；

  具體來(lái)說(shuō)分別是：

  1. hostName 變量不應(yīng)該被重復(fù)使用，尤其當(dāng)這兩次使用時(shí)的含義還不同的時(shí)候；
  2. 將獲取 hostName 的代碼抽離出來(lái)，定義為 getLastfieldOfHostName() 函數(shù)；
  3. 刪除代碼中的魔法數(shù)；
  4. generate() 函數(shù)中的三個(gè) if 邏輯重復(fù)了，且實(shí)現(xiàn)過于復(fù)雜，我們要對(duì)其進(jìn)行簡(jiǎn)化；
  5. 對(duì) IdGenerator 類重命名，并且抽象出對(duì)應(yīng)的接口。

• 第二輪：提高可測(cè)試性；

  具體來(lái)說(shuō)分別是：

  1. generate() 函數(shù)定義為靜態(tài)函數(shù)，會(huì)影響使用該函數(shù)的代碼的可測(cè)試性，需要改為非靜態(tài)函數(shù)；
  2. generate() 函數(shù)的代碼實(shí)現(xiàn)依賴運(yùn)行環(huán)境（本機(jī)名）、時(shí)間函數(shù)、隨機(jī)函數(shù)測(cè)試性不好需要進(jìn)行對(duì)相應(yīng)邏輯進(jìn)行拆分和封裝，具體來(lái)下：1、從 getLastfieldOfHostName() 函數(shù)中，將邏輯比較復(fù)雜的那部分代碼剝離出來(lái)，定義為 getLastSubstrSplittedByDot() 函數(shù)；2、將 generateRandomAlphameric() 和 getLastSubstrSplittedByDot() 這兩個(gè)函數(shù)的訪問權(quán)限設(shè)置為 protected。這樣做的目的是，可以直接在單元測(cè)試中通過對(duì)象來(lái)調(diào)用兩個(gè)函數(shù)進(jìn)行測(cè)試。3、給 generateRandomAlphameric() 和 getLastSubstrSplittedByDot() 兩個(gè)函數(shù)添加@VisibleForTesting。告訴其他人說(shuō)，這兩個(gè)函數(shù)本該是 private 訪問權(quán)限的，之所以提升訪問權(quán)限到 protected，只是為了測(cè)試，只能用于單元測(cè)試中。
  3. 針對(duì)異常和特殊值進(jìn)行適當(dāng)處理，拋出調(diào)用方理解的異常。

• 第三輪：編寫完成UT，寫單元測(cè)試的時(shí)候，測(cè)試對(duì)象是函數(shù)定義的功能，而非具體的實(shí)現(xiàn)邏輯；

  針對(duì)generate()函數(shù)可以有三個(gè)不同功能的定義；

  • 如果我們把 generate() 函數(shù)的功能定義為：“生成一個(gè)隨機(jī)唯一 ID”，那我們只要測(cè)試多次調(diào)用 generate() 函數(shù)生成的 ID 是否唯一即可。
  • 如果我們把 generate() 函數(shù)的功能定義為：“生成一個(gè)只包含數(shù)字、大小寫字母和中劃線的唯一 ID”，那我們不僅要測(cè)試 ID 的唯一性，還要測(cè)試生成的 ID 是否只包含數(shù)字、大小寫字母和中劃線。
  • 如果我們把 generate() 函數(shù)的功能定義為：“生成唯一 ID，格式為：{主機(jī)名 substr}-{時(shí)間戳}-{8 位隨機(jī)數(shù)}。在主機(jī)名獲取失敗時(shí)，返回：null-{時(shí)間戳}-{8 位隨機(jī)數(shù)}”，那我們不僅要測(cè)試 ID 的唯一性，還要測(cè)試生成的 ID 是否完全符合格式要求。

  總結(jié)：UT如何寫，關(guān)鍵看你如何定義函數(shù)。

• 第四輪：添加完整注釋，注釋主要就是寫清楚：做什么、為什么、怎么做、怎么用，對(duì)一些邊界條件、特殊情況進(jìn)行說(shuō)明，以及對(duì)函數(shù)輸入、輸出、異常進(jìn)行說(shuō)明。；

優(yōu)化結(jié)果

/**
 * IdGenerator接口 重構(gòu)一 提高代碼可擴(kuò)展性
 */
public interface IdGenerator {
    public String generate() throws IdGenerationFailureException;
}
/**
 * LogTraceIdGenerator接口 重構(gòu)一 提高代碼可擴(kuò)展性
 */
public interface LogTraceIdGenerator extends IdGenerator{
}
/**
 * 用于生成隨機(jī) ID 的 Id 生成器。//重構(gòu)四：添加注釋
 *
 *
 * 此類生成的 ID 不是絕對(duì)唯一的，
 * 但重復(fù)的可能性非常低。
 */
@Slf4j
public class RandomIdGenerator implements LogTraceIdGenerator{//重構(gòu)一 提高代碼可擴(kuò)展性
    /**
     * 生成隨機(jī) ID。只有在極端情況下，才能生成重復(fù)ID。
     *
     * @return隨機(jī) ID
     */
    @Override
    public String generate() throws IdGenerationFailureException {//重構(gòu)一
        String substrOfHostName = null;
        try {
            substrOfHostName = getLastfieldOfHostName();
        } catch (UnknownHostException e) { //重構(gòu)二：封裝新異常繼續(xù)拋出
           throw new IdGenerationFailureException("host name is empty.");
        }
        //if(substrOfHostName == null || substrOfHostName.isEmpty()){//重構(gòu)二：異常處理
        //    throw new IdGenerationFailureException("host name is empty.");
        //}
        long currentTimeMillis = System.currentTimeMillis();

        String randomString = generateRandomAlphameric(8);//重構(gòu)二 提供代碼可測(cè)試性
        String id = String.format("%s-%d-%s", substrOfHostName, currentTimeMillis, randomString);
        return id;
    }

    /**
     * 獲取本地主機(jī)名和
     * 提取由分隔符 '.' 拆分的名稱字符串的最后一個(gè)字段。
     *
     * @return 主機(jī)名的最后一個(gè)字段。如果未獲取主機(jī)名，則返回 null。
     */
    private String getLastfieldOfHostName() throws UnknownHostException {// 重構(gòu)一
        String substrOfHostName = null;
        //try {
        String hostName = InetAddress.getLocalHost().getHostName();

        //重構(gòu)二 NULL判斷
        if (hostName == null || hostName.isEmpty()) {
            throw new UnknownHostException();
        }
        //重構(gòu)二 邏輯拆分
        substrOfHostName = getLastSubstrSplittedByDot(hostName);
        //String[] tokens = hostName.split("\\.");
        //substrOfHostName = tokens[tokens.length -1];

        //}catch (Exception e){////重構(gòu)二：異常處理，直接拋出
        //    log.warn("Failed to get the host name.",e);
        //}
        return substrOfHostName;

    }

    /**
     * 獲取 {@hostName} 的最后一個(gè)字段，該字段由 delemiter '.' 拆分。
     *
     * @param hostName 主機(jī)名不應(yīng)為空
     * @return {@hostName} 的最后一個(gè)字段。如果 {@hostName} 為空字符串，則返回空字符串。
     */
    @VisibleForTesting //重構(gòu)二
    protected String getLastSubstrSplittedByDot(String hostName) {//重構(gòu)二 提供代碼可測(cè)試性
        //重構(gòu)二：NULL 判斷，如果傳入NULL 則拋運(yùn)行異常
        if(hostName == null || hostName.isEmpty()){
            throw new IllegalArgumentException("host name is empty");
        }
        String[] tokens = hostName.split("\\.");
        String substrOfHostName = tokens[tokens.length -1];
        return substrOfHostName;
    }

    /**
     * 生成隨機(jī)字符串，其中
     * 僅包含數(shù)字、大寫字母和小寫字母。
     *
     * @param length 長(zhǎng)度應(yīng)不小于0
     * @return 隨機(jī)字符串 如果 {@length} 為 0 則返回空字符串
     *
     */
    @VisibleForTesting //重構(gòu)二
    protected String generateRandomAlphameric(int length) {//重構(gòu)一 復(fù)雜邏輯拆分 //重構(gòu)二 為了提高測(cè)試性，將private 改為 protected
        if(length < 0){ //重構(gòu)二 邊界處理
            throw new IllegalArgumentException("parameter length is illegal.");
        }

        char[] randomChars = new char[length];
        int count = 0;
        Random random = new Random();
        while (count < length){
            int maxAscii = 'z';
            int radomAscii = random.nextInt(maxAscii);
            //重構(gòu)一 提高代碼可讀性
            boolean isDigit = radomAscii >= '0' && radomAscii < '9';
            boolean isLowercase = radomAscii >= 'a' && radomAscii < 'z';
            boolean isUppercase = radomAscii >= 'A' && radomAscii < 'Z';
            if(isDigit || isLowercase || isUppercase){
                randomChars[count] = (char) radomAscii;
                count++;
            }
        }
        return new String(randomChars);
    }
}

/**
 * RandomIdGeneratorTest類 重構(gòu)三：編寫UT
 */
public class RandomIdGeneratorTest {

    @Test
    public void testGetLastSubstrSplittedByDot(){
        RandomIdGenerator generator = new RandomIdGenerator();
        String actualSubstr = generator.getLastSubstrSplittedByDot("field1.field2.field3");
        Assert.assertEquals("field3",actualSubstr);

        actualSubstr = generator.getLastSubstrSplittedByDot("field1");
        Assert.assertEquals("field1",actualSubstr);

        actualSubstr = generator.getLastSubstrSplittedByDot("field1#field2#field3");
        Assert.assertEquals("field1#field2#field3",actualSubstr);
    }

    @Test
    public void testGetLastSubstrSplittedByDot_nullOrEmpty(){
        RandomIdGenerator generator = new RandomIdGenerator();
        String actualSubstr = generator.getLastSubstrSplittedByDot("");
        Assert.assertEquals("",actualSubstr);

        actualSubstr = generator.getLastSubstrSplittedByDot(null);
        Assert.assertNull(actualSubstr);
    }

    @Test
    public void testGenerateRandomAlphameric(){
        RandomIdGenerator generator = new RandomIdGenerator();
        String randomString = generator.generateRandomAlphameric(8);
        Assert.assertNotNull(randomString);
        Assert.assertEquals(8,randomString.length());
        for (char s : randomString.toCharArray()) {
            Assert.assertTrue((s >= '0' && s < '9') || (s >= 'a' && s < 'z') || (s >= 'A' && s < 'Z'));
        }
    }

    @Test
    public void testGenerateRandomAlphameric_lengthEqualsOrLessThanZero(){
        RandomIdGenerator generator = new RandomIdGenerator();
        String randomString = generator.generateRandomAlphameric(0);
        Assert.assertEquals("",randomString);

        randomString = generator.generateRandomAlphameric(-1);
        Assert.assertNull(randomString);
    }

    /**
     * generate() 函數(shù)的功能定義為：“生成唯一 ID，格式為：{主機(jī)名 substr}-{時(shí)間戳}-{8 位隨機(jī)數(shù)}
     */
    @Test
    public void testGenerate(){
        RandomIdGenerator generator = new RandomIdGenerator();
        String id1 = generator.generate();
        Assert.assertNotNull(id1);
        Assert.assertTrue(id1.length() > 0);
        for (char c: id1.toCharArray()) {
            Assert.assertTrue((c == '-')||(c >= '0' && c < '9') || (c >= 'a' && c < 'z') || (c >= 'A' && c < 'Z'));

        }
        String id2 = generator.generate();
        Assert.assertNotNull(id1);
        Assert.assertTrue(id1.length() > 0);
        for (char c: id2.toCharArray()) {
            Assert.assertTrue((c == '-')||(c >= '0' && c < '9') || (c >= 'a' && c < 'z') || (c >= 'A' && c < 'Z'));

        }

        Assert.assertNotEquals(id1,id2);
    }

    /**
     * generate() 函數(shù)在主機(jī)名獲取失敗時(shí)，返回：null-{時(shí)間戳}-{8 位隨機(jī)數(shù)}”,測(cè)試生成的 ID 是否完全符合格式要求。
     */
    @Test
    public void testGenerate_withoutSubHostName(){
        RandomIdGenerator generator = new RandomIdGenerator();
        generator.getLastSubstrSplittedByDot(null);

        String id = generator.generate();
        Assert.assertNotNull(id);
        Assert.assertTrue(id.length() > 0);
        String[] split = id.split("-");
        Assert.assertTrue(split[0] == null);
        Assert.assertTrue(System.currentTimeMillis() == Long.valueOf(split[1]));
        for (char c: split[2].toCharArray()) {
            Assert.assertTrue((c >= '0' && c < '9') || (c >= 'a' && c < 'z') || (c >= 'A' && c < 'Z'));

        }
    }
}