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

April 08, 2020

這篇文章討論《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) - 避免過度耦合的訊息鏈

jyt0532's Blog

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

一般狀況

相關文章

jyt0532's Blog

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

一般狀況

相關文章

程式碼的氣味和啟發(一) 03 Apr 2020

無瑕的程式碼 - 單元測試 15 Mar 2020

無瑕的程式碼 - 物件及資料結構 15 Mar 2020