cleanCode

程式碼的氣味和啟發(二)

這篇文章討論 《Clean Code》 裡面收官的章節 - 程式碼的氣味和啟發 以及《重構 - 改善既有程式的設計》裡第三章 - 程式碼的壞味道

圖片以及程式碼來源自Clean Code以及重構 - 改善既有程式的設計

一般狀況

G1 - 同份原始檔存在多種語言: 一個Source File就是一個語言 有超過一個語言就會讓人困惑

G2 - 明顯該有的行為未被實現: 比如一個將星期幾轉成enum的函式

Day day = DayDate.StringToDay(String dayName);

你可能會預期輸入 “Monday” 會回傳 Day.MONDAY 但其他使用者可能會覺得 “monday”, “MON”, “mon”, “MONDAY”等等都要回傳正確的值 如果你沒有全部實作的話 會失去大家對這個函式的信賴

G3 - 邊界上的不正確行為: 為你的演算法在所有情況都寫好測試 不要依賴直覺

G4 - 無視安全規範: 比如手動控管serialVersionUID 或是把Compiler的警告關掉 都是很危險的事

G5 - 重複的程式碼: 本書最重要的規範之一 要非常認真看待

如果你看到相同的程式碼 代表在過去發生了該進行抽象而未進行抽象的情形

常見的情況如下

1.如果兩個函式有一樣的程式碼 就提取這些一樣的程式碼到新的函式 讓人家呼叫

2.如果兩個subclass有一樣的程式碼 那可以提取這些程式碼到parent class

3.那如果兩個subclass只是有類似的程式碼 比如說演算法有一點點差異呢? 恩你懂的 Template Method

G6/7 - 在錯誤抽象層次上的程式碼: 要建立能劃分 高層次的一般概念低層次細節概念的抽象模型 我們想把高層次的概念都放在base class 低層次的概念都放在derived class

來看一個可愛的Stack抽象

public interface Stack {
  Object pop();
  void push(Object o); 
  double percentFull();
}

有沒有哪個函式看起來怪怪?

沒錯 percentFull

因為不是所有的Stack都有 多滿 的概念 這種東西就應該放在比如BoundedStack之類的衍生類別裡

G8 - 過多的資訊: 限制類別和模組中曝露的介面數量 當一個類別擁有的方法數量越少越好 當一個函式知道的變數越少越好 當一個類別擁有的實體變數越少越好

專心的在讓你的介面tight and small. Help keep coupling low by limiting information.

G9 - 被遺棄的程式碼: 比如永遠不會跑到的if區 或是永遠不會拋出例外的catch區 這些區塊永遠不會跑到 但若一直放著 久了就會有味道

G10 - 垂直分隔: 變數和函式應該要定義在靠近使用的地方 比如說

1.區域變數應該要在第一次被使用的位置的正上方 垂直距離越短越好

2.私有函式應該要被定義在第一次被呼叫的地方的下面 讓人可以馬上找到

G11/G24 - 不一致性: 不論是命名還是其他事項 都應該遵從一樣的慣例

G12 - 雜亂的程式: 用不到的東西 不需要的東西 都全部拿掉 比如default constructor就可以直接拿掉

G13 - 人為的耦合(artificial coupling): 比如說把enum或是某個static變數宣告在某個類別裡 這樣會迫使enum的使用者必須了解整個類別

這種人為的刻意把兩個東西的關聯性變強 都應該避免

G14(3.7) - 特色留戀(Feature Envy): 一個類別的方法 應該只對同一個類別的變數和函式感興趣 而不是對其他類別的感興趣

來看個例子

public class HourlyPayCalculator {
  public Money calculateWeeklyPay(HourlyEmployee e) {
    int tenthRate = e.getTenthRate().getPennies();
    int tenthsWorked = e.getTenthsWorked();
    int straightTime = Math.min(400, tenthsWorked);
    int overTime = Math.max(0, tenthsWorked - straightTime); 
    int straightPay = straightTime * tenthRate;
    int overtimePay = (int)Math.round(overTime*tenthRate*1.5);
    return new Money(straightPay + overtimePay); }
}

嗯… 這明明就是在HourlyPayCalculator裡面的函式 但卻一直去跟HourLyEmployee要資料 這可能就代表calculateWeeklyPay這個函式本身就該存在HourlyEmployee

G15 - 選擇性參數: 如同F3(旗標參數)

G16 - 模糊的意圖: 我們需要程式可以盡可能地具有表達力 跨行的表達或是匈牙利命名都會讓人困惑

public int m_otCalc() { 
  return iThsWkd * iThsRte +
    (int) Math.round(0.5 * iThsRte * 
      Math.max(0, iThsWkd - 400)
    ); 
}

這什麼鬼 原本2秒可以看懂的程式變成要花10秒

G17 - 錯誤的職責: 軟體工程師能夠做的最重要決定之一 就是應該把程式碼放在哪

比如說PI這個常數應該放在Math裡 還是Trigonometry裡 還是Circle裡?

有時候我們會用一點小聰明來把程式碼放在我們覺得方便使用的地方 但這並不是對讀者來說最直觀的地方

G18 - 不適當的靜態宣告: 我們喜歡把一些常用的函式當作靜態函式來用 比如

Math.max(double a, double b);

這是很合理的 畢竟我們不想要每次需要比較的時候需要new一個物件

new Math().max(a, b)

或是

a.max(b)

這些都不太實用 但也不是所有東西都該宣告能靜態函數 比如說

HourlyPayCalculator.calculatePay(employee, overtimeRate)

乍看之下合理 但是仔細想一下 我們應該很有可能 遲早會需要support這個函式的polymorphism 比如第一個參數如果給進employee的子類 我們也要會算

這樣的話 每當我們需要support一個子類 我們就得把這個子類的calculatePay加進子類的靜態函式 可想而知 重複性太高

總而言之 你應該偏向使用非靜態函式 當你真的需要把它變成靜態的時候 先考慮你需不需要在未來支持多型

G19 - 使用具解釋性的變數: 就是變數命名要清楚

G20 - 函數名稱要說到做到

Alt text

如果你必須仔細看這個函式的實作 才知道它到底做了什麼 那你應該取個好點的名稱

G21 - 暸解演算法: 你必須先瞭解你要實作的算法 再去寫實作的函式

G22 - 讓邏輯相依變成實體相依: 其實跟G17很像 就是當兩個模組有依賴關係的時候 哪個模組要負責那些資料要跟清楚

G23(3.10, 9.6)/G27 - 用多型取代if/else 或 switch: 當你在物件導向的語言看到switch 很有可能可以用多型來取代

class Employee {
 int payAmount() {
   switch (getType()) {
     case EmployeeType.ENGINEER:
       return _monthlySalary;
     case EmployeeType.SALESMAN:
       return _monthlySalary + _commission;
     case EmployeeType.MANAGER:
       return _monthlySalary + _bonus;
     default:
       throw new RuntimeException("Incorrect Employee");
   }
 }
}

當你看到這個 就可以用可愛的Strategy來替換

class Salesman{
  int payAmount(Employee emp) {
    return emp.getMonthlySalary() + emp.getCommission();
  }
}
class Manager{
  int payAmount(Employee emp) {
    return emp.getMonthlySalary() + emp.getBonus();
  }
}

別忘了把base class的函式改成抽象函式

class EmployeeType{
 abstract int payAmount(Employee emp)
}

G25 - 用帶有名稱的常數取代Magic number: 比如用SECOND_PER_DAY而不是86400

G26 - 要精確: 模稜兩可和不精確的程式碼 是由懶惰以及不一致所造成的 比如

1.單純用浮點數來代表貨幣利率(幾乎是犯罪行為)

2.因為不想處理同步問題所以不用lock或是transaction management

3.明明宣告List就夠了你卻硬要宣告為ArrayList來過度限制

4.讓所有變數都沒有access modifier(直接變成package protected)

當你在程式碼中下任何決定時 都要有充足的理由 不要得過且過

G28 - 把條件判斷封裝

比如這個判斷

if (shouldBeDeleted(timer))

就比這個清楚很多

if (timer.hasExpired() && !timer.isRecurrent())

G29 - 避免否定的條件判斷: 否定的判斷比肯定的難了解

你自己來看看這兩個哪個需要讀懂的時間少一點

if (buffer.shouldCompact())
if (!buffer.shouldNotCompact())

G30 - 函式應該只做一件事

我們來看看付錢給員工的函式

public void pay() {
  for (Employee e : employees) {
    if (e.isPayday()) {
      Money pay = e.calculatePay(); 
      e.deliverPay(pay);
    } 
  }
}

這個函式做了三件事 檢查所有員工 檢查哪些員工應該拿錢 給錢 那我們就應該分成三個函式寫

public void pay() {
  for (Employee e : employees)
    payIfNecessary(e); 
}
private void payIfNecessary(Employee e) { 
  if (e.isPayday())
    calculateAndDeliverPay(e); 
}
private void calculateAndDeliverPay(Employee e) { 
  Money pay = e.calculatePay(); 
  e.deliverPay(pay);
}

G31 - 不應該隱藏時序耦合: 不要為了讓函式簡單 而隱藏了函式之間的依賴關係

public class MoogDiver{
  Gradient gradient;
  List<Spline> splines;
  public void dive(String reason){
    saturateGradient();
    reticulataSplines();
    diveForMoog(reason);
  }
}

當我們dive的時候 一定需要按照這三個順序 saturateGradient -> reticulataSplines -> diveForMoog

你程式這樣寫很合理 但這可能讓任何其他的函式 用任何他想用的順序呼叫 你身為這段程式碼的擁有者 無法保證被呼叫的順序

當你函式有時間順序需求的時候 你可以這麼搞

public class MoogDiver{
  Gradient gradient;
  List<Spline> splines;
  public void dive(String reason){
    Gradient gradient = saturateGradient();
    List<Spline> splines = reticulataSplines(gradient);
    diveForMoog(splines,reason);
  }
}

這樣就會逼著一定要按照順序了

G32 - 不要隨意: 你的程式碼的每個地方的結構都要清楚且一致 當你只要某個地方不一致 看的人或改的人就不會覺得保持一致很重要 就會越改越亂

G33 - 封裝邊界條件

當你要處理邊界條件的時候 可能會有這種情況

if(level + 1 < tags.length) {
  parts = new Parse(body, tags, level + 1, offset + endTag);
  body = null; 
}

這兩個+1就很惱人 把他用另一個變數包起來

int nextLevel = level + 1; 
if(nextLevel < tags.length) {
  parts = new Parse(body, tags, nextLevel, offset + endTag);
  body = null; 
}

好看多了

G34 - 函式內容應該下降抽象層次一層: 在一個函式裡的statement 都應該屬於同一個層次的抽象 哪一層呢 就是這個函式本身的低一層

比如說一個兩級抽象深的函式 裡面的內容就只能是三級抽象深的表達式 當你要使用四級抽象深的表達式 你需要再呼叫一個函式

上個例子 如果size > 0 就把hr(水平線)多一個size attribute

public String render() throws Exception {
  StringBuffer html = new StringBuffer("<hr"); 
  if(size > 0)
    html.append(" size=\"").append(size + 1).append("\""); 
  html.append(">");
  return html.toString(); 
}

這個函式混雜了至少兩種以上的抽象層次 一種是水平線的粗細概念 一種是水平線的syntax概念

希望看到這裡的你 已經在心中有些sense 這兩個概念不該是同一層的

重構之後這這樣

public String render() throws Exception {
  HtmlTag hr = new HtmlTag("hr"); 
  if (size > 0)
    hr.addAttribute("size", hrSize(size)); 
  return hr.html();
}
private String hrSize(int height) {
  int hrSize = height + 1;
  return String.format("%d", hrSize); 
}

這個重構把水平線應該要多粗 跟 如何畫粗水平線的概念分開 如果所有render()裡面的表達式都是第n層深的抽象的話 那hrSize()裡面的表達式都是第n+1層抽象

劃分抽象程式概念 是進行函式重構時 最重要的行為之一

G35 - 可以configure的資料放在高階層次

如果你有一個常數 比如說有預設值或是設定值 是configurable的 那就要擺在高階的抽象層次上 不要藏到低層次的函式裡

G36(3.15) - 避免過度耦合的訊息鏈