งานที่ผมทำก่อนหน้านี้ไม่ใช่ 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 หยิบมาแบบมั่วๆ)
