การ Code review เป็นหนึ่งในกิจกรรมที่ทุกทีมทำ แต่ทีมส่วนใหญ่ทำได้แค่ "ผ่านๆ" กดผ่านเพราะเชื่อใจคนเขียน หรือ ไม่ก็จับผิดจุกจิกจนคนส่ง PR เริ่มกลัวการรีวิว ทั้ง 2 แบบพลาดประเด็นเดียวกัน คือ มองว่า review เป็นด่านตรวจ ทั้งที่จริงมัน คือ เครื่องมือกระจายความรู้ และ ยกระดับคุณภาพของทั้งทีม

บทความนี้จะไล่ว่าในทางปฏิบัติเราควรทำอะไรบ้าง ตั้งแต่ก่อนเปิดไฟล์ diff ไปจนถึงตอนพิมพ์ comment

เรา review ไปเพื่ออะไรกันแน่

ก่อนจะลงรายละเอียด ต้องตกลงเป้าหมายกันก่อน เพราะมันกำหนดว่าเราจะรีวิวยังไง เป้าหมายของ code review ไม่ใช่การหาคนผิด แต่ คือ 3 อย่าง

  1. จับปัญหาให้เจอตั้งแต่ก่อนขึ้น production
  2. ทำให้โค้ดในระบบเข้าใจและดูแลต่อได้ในระยะยาว
  3. ให้คนในทีมเรียนรู้จากกันและกัน ทั้งคนรีวิวและคนถูกรีวิว

เมื่อตั้งเป้าแบบนี้ มาตรฐานของ "รีวิวที่ดี" จะเปลี่ยนไปทันที มันไม่ใช่รีวิวที่คอมเมนต์เยอะที่สุด แต่ คือ review ที่ทำให้ code ดีขึ้น โดยที่ความสัมพันธ์ในทีมยังดีอยู่

ก่อนเริ่มรีวิว: เข้าใจบริบทก่อน

หลายๆ ครั้ง พบว่า คน review มักจะกระโดดเข้าไปอ่าน code เลย ซึ่งเป็นวิธีที่ผิด อย่าเพิ่งกระโดดเข้าไปอ่านบรรทัดแรกแล้ว เริ่ม comment ให้เราใช้เวลาสักครู่ ทำความเข้าใจว่า PR นี้ต้องการแก้อะไร ดูจากชื่อ PR คำอธิบาย และ ticket ที่ผูกไว้

ถ้าอ่านคำอธิบายแล้วยังไม่เข้าใจว่ากำลังจะแก้ปัญหาอะไร นั่นแหละ คือ comment แรกที่ควรฝากไว้

เพราะถ้าคน review ยังงง คนที่มาอ่าน git history อีกหกเดือนข้างหน้าก็จะงงเหมือนกัน

อีกเรื่องที่ช่วยได้มาก คือ ขนาดของ PR ถ้าเจอ PR ที่แก้ไป 40 ไฟล์ 2,000 บรรทัด อย่าฝืนรีวิวรวดเดียว เพราะงานวิจัยและประสบการณ์ตรงของผม บอกได้เลยว่า คุณภาพการ review จะตกฮวบเมื่อ diff ใหญ่เกินไป แค่ 5-10 ไฟล์ เราก็พร้อมที่จะข้ามมันไปแล้ว

การ review โค้ดได้อย่างมีคุณภาพ จะอยู่ที่ราวๆ 200–400 บรรทัดต่อรอบเท่านั้น เกินกว่านั้นสายตาจะเริ่มไถผ่าน สิ่งที่ควรทำ คือ ขอให้เจ้าของ PR ซอยงานให้เล็กลง ดีกว่ากดผ่านทั้งก้อนแบบไม่ได้ดูจริง

อีกเรื่องหนึ่ง คือ 1 PR = 1 ปัญหาที่ต้องการแก้ไข

ตัวอย่าง PR Template ที่เราใช้ 

# Problems
ปัญหาที่ต้องการแก้ไข

# Solutions
วิธีการแก้ไขที่เลือกใช้

# Changes
- เปลี่ยนแปลงในส่วนไหนบ้าง

# Requests
- อยากให้ช่วยอะไรเป็นพิเศษ

PR Template

รีวิวอะไรบ้าง: เรียงตามลำดับความสำคัญ

จุดพลาดที่พบบ่อย คือ เรามันจะ review แบบสุ่ม เห็นอะไรก่อนก็ comment อันนั้น ทำให้ไปเสียเวลากับเรื่องจุกจิกอย่างการเว้นวรรค แต่มองข้าม bug logic ที่อยู่ถัดไปอีก 2 บรรทัดก็ได้

วิธีที่ดีกว่า คือ ไล่ตามลำดับความสำคัญนี้:

  1. ผลทดสอบ (Tests): ถ้าเราเขียน automation test ก็ให้เริ่มจากการ run test เพื่อทดสอบก่อนเลย เพราะถ้า test แล้วไม่ผ่าน ก็ไม่จำเป็นต้องเสียเวลาไป review ต่อ แต่ถ้า test ผ่านแล้ว ให้เราตรวจสอบว่ามี test ครอบคลุม logic ที่สำคัญพอไหม test ที่เขียนมาจะต้องทดสอบใกล้เคียงพฤติกรรมจริง ไม่ใช่แค่เขียนให้ผ่าน coverage ดังนั้นการเขียนอธิบาย test case จงสำคัญมากๆ เพราะถ้าเราอ่าน test case แล้วไม่รู้เรื่องมันก็ยากในการทำความเข้าใจ
  2. ความถูกต้อง (Correctness): โค้ดทำในสิ่งที่ requirement ต้องการจริงไหม เข้าใจ edge case ครบหรือเปล่า ค่า null ค่าว่าง ค่าติดลบ จัดการไว้ยัง รวมถึงการใช้งาน ตัวแปร function ไปจนถึง concept ของแต่ละภาษาถูกไหม นี่ คือ สิ่งที่สำคัญที่สุดและควรดูก่อนเสมอ
  3. ความปลอดภัย (Security): มี SQL injection, XSS ไหม มี secret หรือ API key หลุดมาใน source ไหม input ที่รับจากผู้ใช้ผ่าน validation หรือยัง เรื่องพวกนี้ถ้าหลุดขึ้น production แล้วเจ็บหนักกว่า bugs ทั่วไปมาก
  4. ประสิทธิภาพ (Performance): มี N+1 query ไหม มี re-render ที่ไม่จำเป็นในฝั่ง frontend ไหม loop ซ้อน loop บนข้อมูลก้อนใหญ่หรือเปล่า (Big O) ลำดับการเขียนคำสั่ง เป็นต้น แต่ระวังอย่า optimize ก่อนเวลา ถ้ายังไม่มีหลักฐานว่าช้าจริง อย่าเพิ่งบังคับให้แก้
  5. ความอ่านง่าย (Readability): คนอื่นอ่านแล้วเข้าใจไหม โดยไม่ต้องพึ่ง comment เยอะๆ ชื่อตัวแปรและฟังก์ชันสื่อความหมายไหม (getUserById ดีกว่า getData) ฟังก์ชันหนึ่งทำหลายอย่างเกินไปหรือเปล่า
  6. ความดูแลต่อได้ (Maintainability): ถ้าอีก 3 เดือนต้องกลับมาแก้ตรงนี้ จะแก้ง่ายไหม code ผูกกันแน่นเกินไปจนแก้จุดเดียวพังหลายจุดหรือเปล่า

ลำดับนี้สำคัญ เพราะถ้าเวลารีวิวมีจำกัด คุณอยากใช้มันไปกับข้อ 1, 2 และ 3 มากกว่าข้อ 5

เขียน comment ยังไงให้คนอยากแก้ ไม่ใช่อยากเถียง

เนื้อหาที่ถูกต้องแต่ส่งด้วยน้ำเสียงที่ผิด ก็ทำให้รีวิวล้มเหลวได้ การสื่อสารออกไปจึงสำคัญไม่แพ้กัน โดยหลักที่ใช้ได้จริงมีไม่กี่ข้อ เช่น

คอมเมนต์ที่ตัว code ไม่ใช่ที่ตัวคน

ถ้าเราพูดว่า...

"ตรงนี้ถ้า input เป็น null จะ throw นะ"

แทนที่จะพูดว่า...

"คุณลืมเช็ค null อีกแล้ว"

ประโยคแรกชวนคุยเรื่องโค้ด ประโยคหลังชวนให้ตั้งการ์ด

เมื่อมีการตั้งการ์ดแล้ว การ review ก็จะกลายเป็นการป้องกันตัว ไปโดยปริยาย

ถามเป็นคำถาม เมื่อคุณไม่แน่ใจ

บ่อยครั้งคนเขียนมีเหตุผลที่เราไม่เห็น การถามว่า...

"เลือกทำแบบนี้เพราะอะไรเหรอ มีเคสที่ต้อง...ไหม"

ได้ผลดีกว่าการสั่งให้แก้ตรงๆ เพราะบางทีคำตอบจะทำให้ เรา เป็นฝ่ายเข้าใจอะไรใหม่ก็ได้

แยกให้ชัดว่าอันไหนต้องแก้ อันไหนแค่เสนอ

ใช้ป้ายกำกับให้เป็นนิสัย เช่น

  • blocker: สำหรับสิ่งที่ต้องแก้ก่อน merge,
  • nit: สำหรับเรื่องเล็กที่แก้หรือไม่แก้ก็ได้
  • question: สำหรับข้อสงสัย

วิธีนี้ช่วยให้เจ้าของ PR รู้ว่าอะไร คือ สิ่งสำคัญจริงๆ ไม่ต้องเดาว่าคอมเมนต์ไหน คือ เงื่อนไขของการผ่าน

ชมของที่ดีด้วย

เรื่องนี้หลายๆ พวกเรามักจะมองข้ามกัน แต่ถ้าเจอวิธีแก้ที่สวย หรือ test ที่เขียนดี ให้เราบอกไปเลย รีวิวไม่ได้มีไว้หาแต่ข้อเสีย และ การชมทำให้คนกล้าและมีกำลังใจในเขียน code ดีๆ ต่อไป

สิ่งที่ไม่ควรทำ

มีพฤติกรรมบางอย่างที่ทำให้รีวิวเสียของ ที่เจอบ่อยสุด คือ ...

การไปเถียงเรื่อง style ที่ควรให้เครื่องมือจัดการ เช่น เว้นวรรค ขึ้นบรรทัด single quote หรือ double quote

เรื่องพวกนี้ต้องยกให้ linter กับ formatter (เช่น ESLint, Prettier) ตัดสินอัตโนมัติ อย่าเอาสมองคนไปเปลืองกับมัน เพราะทุกนาทีที่ใช้เถียงเรื่อง format คือ นาทีที่ไม่ได้ใช้มองหา bugs จริง

อีกอย่าง คือ bikeshedding – การใช้เวลาส่วนใหญ่ไปกับเรื่องเล็กๆ น้อยๆ ที่ทุกคนมีความเห็นได้ง่าย (ชื่อตัวแปร สีปุ่ม) แล้วมองข้ามการตัดสินใจเชิงสถาปัตยกรรมที่สำคัญมากกว่า

สุดท้าย คือ การ review ด้วย ego — ยืนกรานให้เขียนแบบที่ตัวเองชอบทั้งที่วิธีเดิมก็ถูกต้อง และ อ่านได้ ถ้ามันไม่ผิดและไม่ได้ทำให้แย่ลง หรือ ยังอ่านได้ง่ายอยู่ (Readability) ก็ปล่อยผ่านได้

ในมุมของคนเปิด PR

รีวิวที่ดีเริ่มตั้งแต่ก่อนเปิด PR ด้วยซ้ำ ถ้าเราเป็นคนส่งงาน สิ่งที่ช่วยผู้รีวิวได้มากที่สุด คือ การเขียนคำอธิบาย PR ให้ดี

บอกว่าแก้อะไร ทำไมถึงเลือกวิธีนี้ แก้จุดไหนไปบ้าง และมีจุดไหนที่อยากให้ช่วยดูเป็นพิเศษไหม การใช้เวลา 5 นาทีเขียนคำอธิบาย ประหยัดเวลาผู้ review ไปได้เป็นครึ่งชั่วโมงเลยละ

ก่อนกดขอ review ให้ self-review ตัวเองก่อนอย่างน้อย 1 รอบเสมอ เปิด diff อ่านเหมือนเป็นคนอื่น

ถ้าเราจะเจอ debug log ที่ลืมลบ หรือโค้ดที่ commented out ทิ้งไว้ได้เอง และเวลาได้ comment กลับมา อย่ารับเป็นการโจมตี ถ้าไม่เห็นด้วยก็อธิบายเหตุผล เพราะบทสนทนาที่ดีระหว่าง 2 ฝ่าย คือ หัวใจของ review ไม่ใช่การที่ฝ่ายใดฝ่ายหนึ่งสั่ง แล้วอีกฝ่ายแค่ทำตาม

ทำให้มันยั่งยืน

การ review ที่ดีจะอยู่ได้ก็ต่อเมื่อทีมออกแบบให้มันไม่เป็นภาระ

หลักการสั้นๆ 3 ข้อ คือ

  1. ให้เครื่องมือทำงานที่น่าเบื่อแทน — CI, linter, formatter, test runner ควรจับ error, style และเทสต์ตกให้อัตโนมัติก่อนถึงมือคน คนจะได้โฟกัสกับตรรกะและดีไซน์
  2. PR เล็กและถี่ ดีกว่าใหญ่และนานๆ ครั้ง — เพราะรีวิวง่ายกว่า merge ปลอดภัยกว่า และ feedback วนกลับเร็วกว่า
  3. ตั้งเป้าเวลาตอบสนอง — PR ที่ค้างในคิวสองวันทำให้ทั้งทีมช้าลง ทีมส่วนใหญ่ที่เวิร์กจะพยายามรีวิวให้ได้ภายในวันเดียวกัน

สรุป: เช็กลิสต์ที่เอาไปใช้ได้เลย

ก่อนกดผ่าน PR ถามตัวเองตามนี้:

  • เข้าใจไหมว่า PR นี้แก้อะไรและทำไม
  • ทดสอบและตรวจสอบว่ามีเทสต์คลุม logic สำคัญพอไหม
  • โค้ดทำถูกตาม requirement ครบทุก edge case ไหม
  • มีช่องโหว่ด้านความปลอดภัย หรือ secret หลุดไหม
  • มีปัญหา performance ที่เห็นชัด (N+1, loop หนักๆ) ไหม
  • คนอื่นอ่านแล้วเข้าใจโดยไม่ต้องถามไหม
  • comment ที่ฝากไป แยกชัดไหมว่าอันไหน blocker อันไหนแค่เสนอ
  • ได้ชมของที่ทำดีบ้างหรือยัง

Code review ที่ดีไม่ได้วัดกันที่จับผิดได้กี่จุด แต่วัดกันที่ทีมส่งของได้ดีขึ้น เข้าใจโค้ดของกันและกันมากขึ้น และยังอยากรีวิวงานให้กันในครั้งต่อไป ถ้าทำได้แบบนั้น แปลว่าเราทำถูกทางแล้ว