Month: May 2014

งานที่ผมทำก่อนหน้านี้ไม่ใช่ software engineer หรือ programmer แต่มันมีส่วนที่ได้เขียนโค้ดบ้าง ซึ่งมักเป็นโปรเจค “เครื่องเคียง” ซะเป็นส่วนใหญ่ กล่าวคือ เอ็งทำเสร็จก็ดี (หรือดีมาก) แต่ไม่เสร็จก็ไม่ได้กระทบอะไรกับทีมนักหนา

ดังนั้น การเขียนโค้ดของผมช่วงทำงาน 3.5 ปีแรกมันไม่ได้มีโปรเซสอะไรมาก ค่อนข้างลูกทุ่ง (ภาษาอังกฤษใช้คำว่า “cowboy”) คือ เขียนเอาผลลัพธ์อย่างเดียว เสร็จใช้งานได้ดีตาม requirements เป็นโอเค – Get shit done!

ส่วนหนึ่งที่ไม่เคยทำคือ โค้ดรีวิว (code review)

ดังนั้นการเปลี่ยนแปลงที่สำคัญที่ผมเจอหลังมาเริ่มงานกับ “อากู๋ให้ 5 ดาว” (ให้ห้าดาวนั่นไม่เกี่ยว แต่ถ้าคุณเก็ต แปลว่าคุณแก่มากกกกก) คือการได้สัมผัสการเขียนโค้ดแบบที่ต้องให้คนอื่นรีวิวและเห็นด้วยก่อนจึงจะ submit ได้ ดังนั้นการรีวิวจึงเกิดขึ้นอยู่ตลอดเวลา ไม่ได้ทำวันละครั้งหรืออาทิตย์ละครั้ง

เชื่อหรือไม่ว่า ว่ากลุ่มคนผู้รักการเขียนโค้ดส่วนใหญ่ (รวมตัวเอง)​ มักจะคิดว่าโค้ดของตัวเองดีเลิศประเสริฐศรี สวยงาม ปลอดบั๊ก แต่ก็นั่นแหละ เชื่อหรือไม่(อีก)ว่า การให้ตาที่สอง หรือ สาม สี่ ห้า มาดูด้วย เราจะเห็นอะไรที่เรามองไม่เห็นและเป็นประสบการณ์เรียนรู้ที่ดีเลยทีเดียว ส่วนคนรีวิวก็มีโอกาสได้อ่านโค้ดเราและเรียนรู้อะไรที่ไม่เคยรู้เช่นกัน

ข้างล่างเป็นตัวอย่างเท่าที่จำได้ พอหอมปากหอมคอ ให้เห็นภาพว่าในโค้ดรีวิวเค้าคุยเรื่องอะไรกันบ้าง (แน่นอนว่ามันไม่ใช่โค้ดที่เขียนจริงๆ)​

ตย. 1

public HashMap<Integer, String> getIdToNameMapping(String path) {

ตย. 2

if node.index >= mid and not node.visited:
  return True
else
  return False

ตย. 3

# Count the number of brown dogs and print a list of names.
brown_dogs = [d for d in dogs if d.color == 'brown']
print "There are %s dogs: %s % (len(brown_dogs), ', '.join(brown_dogs))

ตย. 4

class A {
public:
 A { x_ = new B(); }
 ~A { delete x_; }
private:
 B* x_
};

ตย.5

logger.warning(String.format(
  "Could not parse data for item: %s", itemId));

ตย.6

class CoffeeMaker {
  private CoffeeBean bean;
  public CoffeeMaker(CoffeeBean bean) {
    this.bean = bean;
  }
  // ...
}

ตย.7

bool CreateTableFromFile(string path, Table& table);

ผลลัพธ์ที่ได้จากการรีวิว

ตย. 1
ฟังก์ชันมัน return HashMap ซึ่งจริงๆแล้วคนเรียกฟังก์ชัน​(อาจจะ)ไม่ได้สนใจว่ามันจะเป็น Map ประเภทไหน (ขอให้เป็น Map ก็พอ) ดังนั้นเราควรจะ return type ที่มัน general ที่สุด ในที่นี้ก็เป็น Map แทน

public Map<Integer, String> getIdToNameMapping(String path) {

ตย. 2
unnecessary if/else อันนี้เป็นอะไรที่รู้อยู่แก่ใจแต่หลายๆครั้งเขียนโค้ดแก้ๆลบๆ เลยหลุดไปบ้าง ต้องให้คนมาเห็นแล้วเตือน

return node.index >= mid and not node.visited

ตย. 3
ข้อนี้น่าจะเป็นอะไรที่น่าแปลกใจสำหรับเราชาวไทยผู้ภาษาอังกฤษไม่แข็งแรง (ใช่ ไม่ได้เกี่ยวกับโค้ดเลย..) คือมันมีประโยคภาษาอังกฤษที่เรียกว่า ประโยคคำสั่ง (หรือ imperative) และประโยคบอกเล่า (descriptive) ซึ่งชาวต่างชาติเจ้าของภาษาเค้าคงมองสองอันนี้แยกออกจากกันได้ชัดเจน ทีนี้เนี่ย ประโยคที่ใช้เขียนคอมเม้นต์ใน code มันควรจะเป็นประโยคบอกเล่า ดังนั้นโค้ดข้างล่างแค่เติม s ลงไปที่ verb ก็พอ (ให้ตัว code เป็นประธาน)

# Counts the number of brown dogs and prints a list of names.

ตย. 4
มีข้อตกลงกันภายใน (code standard, style หรือ convention อะไรก็แล้วแต่)​ ว่าให้หลีกเลี่ยง raw pointer ดังนั้นตรงนี้มันควรจะเป็น std::unique_ptr แทน เหตุผลประกอบดูได้ใน C++ style guide

class A {
public:
 A { x_.reset(new B()); }
private:
 std::unique_ptr<B> x_
};

ตย.5
บางอย่างก็ขึ้นกับรสนิยมส่วนตัวของ reviewer แต่ละคน อย่างคนนี้มักจะชอบคอมเม้นต์เรื่องการใช้ String.format อย่างฟุ่มเฟือย ในเคสที่มันใช้ + ก็น่าจะเพียงพอ

logger.warning("Could not parse data for item:" + itemId));

ตย.6
เป็นเรื่องของ Java ที่ผมไม่เคยคิดว่าคนอื่นเค้าให้ความสำคัญกัน คือบาง field ที่เป็น final ได้ควรจะเป็น final (อารมณ์เดียวกับเรื่อง const nazi ของ C++)

class CoffeeMaker {
  private final CoffeeBean bean;
  public CoffeeMaker(CoffeeBean bean) {
    this.bean = bean;
  }
  // ...
}

ตย.7
เหมือนจะเป็นเรื่องที่ชาว C++ เค้าเข้าใจตรงกัน ว่าสำหรับ argument ที่เป็น input มันก็ควรจะเป็น const หรือ const reference (จะได้มั่นใจว่าฟังก์ชันที่เรียกไม่ได้แก้ไข input) และ output ก็ควรจะเป็น pointer (เพื่อเป็นสัญญาณว่าชั้นได้ pointer มา ชั้นจะทำอะไรก็ได้) กลายเป็น

bool CreateTableFromFile(const string& path, Table* table);

ตัวอย่างพวกนี้เป็นน้ำจิ้มมากๆ จริงๆแล้วมีปัญหาระดับที่ใหญ่ขึ้นมาอีก เช่น

• ออกแบบ classes มาไม่ดี ต้องกลับไปโละใหม่หมด – อันนี้แก้ได้โดยการไปคุยออฟไลน์กับคนรีวิวก่อน ว่าวางแผนจะทำแบบนี้ๆ มี issue อะไรมั้ย
• แพตช์ที่ส่งไป ใหญ่เกินไป ทำหลายอย่างมากเกินไป – ทำให้รีวิวยาก ใช้เวลานาน ผมคิดว่าประมาณ 500 บรรทัดนี่เริ่มตึงๆแล้ว ดังนั้นพอเสร็จเป็นจุดๆไปก็ควรส่งเข้าไปรีวิวทันที อย่าดองไว้นาน
• เขียนโค้ดแล้วไม่มีเทสต์มาคู่กัน

ทำโค้ดรีวิวก็ใช่ว่าจะไม่มีข้อเสีย ถ้าเจอ reviewer สนใจเรื่องหยุมหยิมมากๆ (ตั้งชื่อตัวแปรไม่ถูกใจ, indent code ที่มันยาวไปหลายบรรทัดไม่เหมือนกัน, วิธีเรียง member ใน class, … !@#$@#$) ก็ทำให้เสียเวลาโดยไม่ได้อะไรกลับมามาก หรือถ้า reviewer ไม่มีเวลา หรืออยู่คนละประเทศ ก็ทำให้เราทำงานช้าลงได้

หลายๆครั้ง เราอาจไม่ต้องการโค้ดรีวิวเลยก็ได้ ยกตัวอย่างเช่น ถ้าคุณทำเว็บ marketing ให้นมตราหมี (ทำไมต้องนมตราหมี)​ ซึ่งแน่ใจว่าใช้งาน 4 เดือนหมด campaign แล้วโยนทิ้งแน่นอน เราจะสนใจ code quality มากเท่าทำงานให้เสร็จทันเวลาหรือไม่? เป็นเรื่องที่ต้องคิด ส่วนบริษัทหลายแห่งที่บังคับให้โค้ดรีวิวเป็นส่วนหนึ่งในการทำงาน คงคิดมาระดับหนึ่งแล้วว่า เวลาที่เสียไปกับการรีวิวคงคุ้มค่ากับการที่ engineer ไม่ต้องมาเสียเวลาปวดหัวทำความเข้าใจโค้ดเน่าๆ

โดยส่วนตัวผมได้เรียนรู้อะไรจากการที่มีคนรีวิวโค้ดให้เยอะมาก โดยเฉพาะคนที่เขียนโค้ดแล้วตั้งใจให้อยู่คู่โลกไปนานๆ (เท่จัง​)​ การทำโค้ดรีวิวคงเป็นสิ่งที่ขาดไม่ได้เลยทีเดียว

(รูปหัวบล็อกเอามาจาก Chromium หยิบมาแบบมั่วๆ)