同步至 medium

看點 6. 求你讓我好讀點

1. 是不是太長，我看的很累呢 ?
2. 裡面是不是一堆 if elase 而且還是內部還有呢 ? 也就是說裡面有很多分支 ( 就是複雜度很高，且難理解 )
3. 是否有基本的 input 與 output 型別定義，讓人知道回傳的東西 ( 我們家有很多地方還是 js )
4. 命名是否好理解 ( 這個在上面有提到 )
5. 不要有魔術數字。

接下來簡單的說說上面的一些我覺得比較可以說的 :

1. 是不是太長，我看的很累呢 ?

這一點有點主觀，所以都會是用建議來看看對方想不想用 extract method 來改改。

2. 裡面是不是一堆 if elase 而且還是巢狀呢 ? 也就是說裡面有很多分支 ( 就是複雜度很高，且難理解 )

ifelse 我覺得不是罪惡，重點是在這兩個 :

• 這個方法是不是有太多情境變化了 ? (SRP)
• 你的寫法是不是讓 ifelse 很難理解。

第一個應該在 SRP 那說過很多次了，就先跳過。

然後第二個，應該不少人都有看果，然後通常會有兩種類型，並且有不同優化法。

首先第一種就是沒辦法提前返回的，通常這種類型就是用設計模式來解決，以下面的範例來看就是使用策略 模式。

// Bad
function getDiscount(price: number, customerType: string): number {
  if (customerType === "VIP") {
    if (price > 500) {
      return price * 0.8;
    } else if (price > 100) {
      return price * 0.85;
    } else {
      return price * 0.9;
    }
  } else if (customerType === "Regular") {
    if (price > 500) {
      return price * 0.9;
    } else if (price > 100) {
      return price * 0.95;
    } else {
      return price * 0.97;
    }
  } else {
    return price;
  }
}

// Good
interface DiscountStrategy {
  applyDiscount(price: number): number;
}

class VIPDiscount implements DiscountStrategy {
  applyDiscount(price: number): number {
    if (price > 500) return price * 0.8;
    if (price > 100) return price * 0.85;
    return price * 0.9;
  }
}

class RegularDiscount implements DiscountStrategy {
  applyDiscount(price: number): number {
    if (price > 500) return price * 0.9;
    if (price > 100) return price * 0.95;
    return price * 0.97;
  }
}

class DiscountContext {
  private strategy: DiscountStrategy;

  constructor(strategy: DiscountStrategy) {
    this.strategy = strategy;
  }

  getDiscount(price: number): number {
    return this.strategy.applyDiscount(price);
  }
}

const customerStrategyMap: Record<string, DiscountStrategy> = {
  VIP: new VIPDiscount(),
  Regular: new RegularDiscount(),
  New: new NewDiscount(),
};

function getDiscount(price: number, customerType: string): number {
  const discountStrategy = customerStrategyMap[customerType];
  const discountContext = new DiscountContext(discountStrategy);
  return discountContext.getDiscount(price);
}

還後還有一種就是可提前返回的，然後通常寫成比較好理解就是提前讓他返回

// Bad
function checkUser(user: { active: boolean, role: string }) {
  if (user) {
    if (user.active) {
      if (user.role === "admin") {
        return "Access granted";
      } else {
        return "Access denied";
      }
    } else {
      return "User not active";
    }
  } else {
    return "No user found";
  }
}

// Good
function checkUser(user: { active: boolean, role: string }) {
  if (!user) return "No user found";
  if (!user.active) return "User not active";
  if (user.role === "admin") return "Access granted";
  return "Access denied";
}

5. 不要有魔術數字

這個有幾種情況，然後我自已都覺得會讓我都需要動腦或是找 code 看一下代表的意思 :

首先第 1 種情境，程式碼內的如下，會讓我花時間想一下 3600000 所代表的時間。

// Bad
function setSessionTimeout() {
  const timeout = 3600000;  // 魔術值
  setTimeout(() => {
    console.log("Session expired");
  }, timeout);
}

// Good
const ONE_HOUR_IN_MS = 60 * 60 * 1000;

function setSessionTimeout() {
  setTimeout(() => {
    console.log("Session expired");
  }, ONE_HOUR_IN_MS);
}

再來第 2 種情境為回傳值送我魔術值，外面使用時我都在通靈你是回傳什麼。

// Bad
function processPayment(amount: number): number {
  if (amount < 0) {
    return -2;  // 魔術值
  }
  if (amount === 0) {
    return -1;  // 魔術值
  }
  // 處理付款
  return 0;
}

// Good
enum PaymentStatus {
  ERROR_NEGATIVE_AMOUNT,
  ERROR_ZERO_AMOUNT,
  PAYMENT_SUCCESS,
}

function processPayment(amount: number): PaymentStatus {
  if (amount < 0) {
    return PaymentStatus.ERROR_NEGATIVE_AMOUNT;
  }
  if (amount === 0) {
    return PaymentStatus.ERROR_ZERO_AMOUNT;
  }
  // 處理付款
  return PaymentStatus.PAYMENT_SUCCESS;
}

最後就是測試時的詭異數字。如下我真的不知道那個 2 是衝啥小，所以我通常會寫的向下面一樣，雖然會耗些資源，但只要 course 不要太大那就還好。

// Bad
describe("CourseService", () => {
  it("should return the correct number of courses", async () => {
    const courses = await CourseService.queryCourses();
    expect(courses.length).to.equal(2);  // 魔術數字 2
  });
});

// Good
describe("CourseService", () => {
  it("should return the correct number of courses", async () => {
    const courses = await CourseService.queryCourses();
    expect(courses.length).to.equal([courseA, courseC]);  // 魔術數字 2
  });
});

看點 7. 不要改變輸入的東西

沒有必要的情況下，儘量不要 input 什麼然後就改變它，這個真的很難追 code，除非你們團隊有統一規定，某些特殊命名的方法有這種功用，例如populateXXX之類的，不然後真的不要用，除了難理解，很多時後會讓後面發生錯誤。

看點 8. 適當的注解，和移除無用的注解

1. workaround 或是特殊的寫法 ( 就是工程看了第一眼都會想為啥這樣寫的概念 ) 有沒有寫注解呢
2. 是不是有一堆沒用的註解了，請砍了它 ( 尤其是 AI 產 code，而不少人都把這個註解留這 )

然後來說說第 2 點。

2. 是不是有一堆沒用的註解了 ( 尤其是 AI 產 code，而不少人都把這個註解留這 )

我知道有不少工程師會對 AI 產的 code 一直噴，但我自已的想法是，只要你有分辦好與壞的能力，那叫 AI 幫我們做事，可以幫我到我們，我自已會覺得沒啥問題 ~ 我覺得比較大的問題是用的人的問題，看過比較不好的幾個情況是會請 AI 生成 PR Description 然後就直接給人了，但是結果 AI 是將每一行 code 都在說明做了啥，但是 code reviewer 的人完全不需要。

還有就是用 AI 生成 code，然後問他為什麼會這樣寫呢，有什麼優點呢 ? 他就說 AI 這樣寫，這種人真的有點感覺是被 AI 用奴役住了 ~ 不是很好 ~ ( 年輕時會說真想打他，但年紀大了就覺得你自已的選擇 ~ 後果自已承擔 )

看點 9. 程式碼風格是否一致

1. 程式碼用的語法版本，以 nodejs 為例我們都統一用 async/await 而不是 promise。
2. 變數名、類別名、資料夾名、檔案名。

只是我發現除非可以自動化檢查，不然還是很難持住，當你進到公司時本來就是 2 ~ 3 種風格時，這就又更難了，因為有時後連我也記不住我們是什麼格式，有時後就參數一下旁邊的檔案，結果剛好就是錯的。

看點 10. TODO 有沒有開票

這個是最近我覺得很重要的東西，因為如果只是寫在程式碼中，而沒有開票放在 backlog 中，最後幾乎整份 code 有超多 todo，然後完全不知道還要不要 todo，因為寫的人都走了。

小結

這篇文章中我大概整理了一下我 code review 時會看的比較篇理解性的幾個點。

• 看點 6. 求你讓我好讀點
• 看點 7. 不要改變輸入的東西
• 看點 8. 適當的注解，和移除無用的注解
• 看點 9. 程式碼風格是否一致
• 看點 10. TODO 有沒有開票

整體來說以我個人的經驗來看，和 10 個工程師問太它們寫的 code 好理解嗎 ? 10 個有 10 個會說自已的 code 很好理解，客觀來說不能說錯，但有幾個前提假設 :

• 看的人的知識量是否和你相同。
• 看的人對你寫的這塊業務 context 是否和你相同。
• 看的人是不是等同於當下寫 code 的你。

根據以上幾點，事實上我覺得不算錯，因為假設上述三個條件下都符合的情況下，我也會覺得他寫的 code 很好理解，就像我上面寫的 magiac number 為 2 這個情境，如果是寫的當下他，當然很清楚知道 2 是正確的，但可惜我不是寫的時後當下你的 ~

這裡我是覺得團隊真的要訂 rule 來維持理解度，不然真的就是一中各表，每個人都覺得自已的好維護，然後每個人接手後，又覺得其它人的很難維護，我覺得問題源都是上面那三個 ~ 然後我覺得覺得還是在於對好理解要用 rule 來維持 ~ 不過這只是我的想法 ~ 也不一定是對。