เป้าหมายหลักของการรีวิวโค้ดคือการหาโค้ดที่บำรุงรักษาได้ยาก
(mathstodon.xyz)- การรีวิวโค้ดใกล้เคียงกับกระบวนการเปิดเผยล่วงหน้าว่าโค้ดส่วนไหนจะ บำรุงรักษาได้ยาก ในภายหลัง มากกว่าจะเป็นขั้นตอนสำหรับจับบั๊กหรือรับประกันความถูกต้องสมบูรณ์
- หากผู้รีวิวอ่านโค้ดแล้วเข้าใจได้ยาก ก็มีความเป็นไปได้สูงว่าผู้ที่ต้องมาดูแลรักษาในอนาคตจะเจอปัญหาเดียวกัน
- การแก้ไขควรทำตอนนี้ที่ผู้เขียนเดิมยังจำบริบทได้อยู่ และการคาดหวังว่าจะหาบั๊กได้อย่างน่าเชื่อถือด้วยการตรวจโค้ดเพียงอย่างเดียวนั้นไม่สมจริง
- แทนที่จะบอกว่า “ลองหาบั๊กดู” การบอกว่า “ลองทำความเข้าใจ แล้วทำเครื่องหมายตรงส่วนที่ไม่เข้าใจ” เป็นงานที่ผู้รีวิวลงมือทำได้จริงมากกว่า
- การรีวิวที่ดีไม่ใช่การพิสูจน์ว่าทุกอย่างสมบูรณ์แบบ แต่เริ่มจากการ ทิ้งบันทึกไว้ในจุดที่ไม่เข้าใจและขอให้ปรับปรุง
เปลี่ยนจุดโฟกัสของการรีวิวโค้ด
- เป้าหมายหลักของการรีวิวโค้ดไม่ใช่ให้ผู้รีวิว หาบั๊ก และไม่ใช่การรับประกันด้วยว่าโค้ดไม่มีบั๊ก
- การคาดหวังว่าจะหาบั๊กได้โดยทั่วไปจากการไล่อ่านโค้ดเพียงอย่างเดียวนั้นใช้ได้ไม่มากในงานจริง
- เพราะฉะนั้น จุดศูนย์กลางของการรีวิวจึงอยู่ที่ “คนอื่นจะกลับมาอ่านและแก้ไขทีหลังได้หรือไม่” มากกว่า “โค้ดนี้ถูกต้องไหม”
โค้ดที่เข้าใจยากคือสัญญาณความเสี่ยงด้านการบำรุงรักษา
- ผู้รีวิวอ่านโค้ดเพื่อพยายามทำความเข้าใจว่าโค้ดทำอะไรและทำงานอย่างไร
- ส่วนที่เข้าใจไม่ได้จะกลายเป็น สัญญาณความเสี่ยง ว่าผู้ดูแลรักษาในอนาคตอาจติดขัดตรงนั้น
- โค้ดแบบนี้ควรรีบแก้ทันทีในช่วงที่ผู้เขียนเดิมยังจำบริบทได้
งานรีวิวที่ทำได้จริง มากกว่าการหาบั๊ก
- คำขออย่าง “ลองหาบั๊กในโค้ดนี้” เป็นงานที่ตัดสินได้ยากว่าทำสำเร็จหรือไม่
- ต่อให้พบบั๊กได้บางส่วน ก็อาจยังมีบั๊กที่ซ่อนอยู่หลุดรอดไป ทำให้ในมุมของผู้รีวิวมีแต่ความล้มเหลวที่ชัดเจน
- ตรงกันข้าม คำขออย่าง “ลองทำความเข้าใจโค้ดนี้ แล้วถ้าเข้าใจไม่ได้ก็ชี้จุดนั้นออกมา” มีเกณฑ์ที่ชัดเจนกว่า
- ไม่จำเป็นต้องเข้าใจทุกอย่างอย่างสมบูรณ์
- แค่บันทึกส่วนที่ตนเองไม่เข้าใจก็พอ
- หากพยายามทำความเข้าใจภาพรวมและทิ้งโน้ตไว้ตรงจุดที่ติดขัด ก็ถือว่าทำหน้าที่ของการรีวิวแล้ว
เกณฑ์การรีวิวในงานจริง
- โค้ดที่ผู้รีวิวไม่เข้าใจอาจนับเป็น สิ่งที่ต้องแก้ไข ได้ในตัวมันเอง
- คอมเมนต์รีวิวไม่ได้มีหน้าที่แค่รายงานบั๊ก แต่ยังช่วยเปิดเผยการอธิบายที่ไม่เพียงพอ ปัญหาเชิงโครงสร้าง และลำดับการไหลที่อ่านยาก
- ภายใต้เกณฑ์นี้ สิ่งสำคัญไม่ใช่การพิสูจน์ความถูกต้องของโค้ด แต่คือการทำให้โค้ดอยู่ในสภาพที่เพื่อนร่วมทีมจะกลับมาอ่านและจัดการต่อได้ในภายหลัง
การพบบั๊กเป็นผลพลอยได้
- ไม่ได้หมายความว่าการรีวิวโค้ดจะหาบั๊กไม่เจอเลย
- อาจพบบั๊กระหว่างการรีวิวได้ แต่ยากที่จะคาดหวังให้มันเป็นวิธีที่ใช้หาบั๊กทั้งหมดหรือบั๊กส่วนใหญ่
- เงื่อนไขความสำเร็จที่สมจริงกว่าคือการตรวจสอบ ความเข้าใจได้ ของโค้ด และปรับปรุงส่วนที่บำรุงรักษายากร่วมกับผู้เขียนเดิมทันที
1 ความคิดเห็น
ความคิดเห็นจาก Hacker News
การรีวิวโค้ดไม่ได้มีจุดประสงค์เดียว การหาว่าโค้ดส่วนไหนดูแลรักษายากก็สำคัญ แต่ไม่ใช่จุดประสงค์เดียว และอาจไม่ใช่จุดประสงค์ที่สำคัญที่สุดด้วย
มันเป็นตัวป้องกันทำให้การ merge โค้ดอันตรายทำได้ยากขึ้น แม้นักพัฒนาหรือ AI จะพาโค้ดออกนอกลู่นอกทาง และคนที่ไม่ได้อยู่ใกล้ปัญหามากเกินไปอาจมองเห็นวิธีที่ดีกว่าหรือปัญหาที่ตกหล่นไปได้
คนที่รู้จักส่วนอื่นของระบบดีกว่าอาจจับปัญหาด้านการทำงานร่วมกันได้ อีกทั้งยังทำให้มีอย่างน้อยหนึ่งคนคุ้นเคยกับโค้ดส่วนนั้น และเป็นโอกาสในการเรียนรู้ทั้งสำหรับผู้เขียนและผู้รีวิว
เรื่องนี้สำคัญมากโดยเฉพาะเมื่อมีความต่างของประสบการณ์ เวลาเมนทอร์พนักงานใหม่ ฉันจะเพิ่มเขาเป็น reviewer ใน PR ของฉันทั้งหมดเพื่อให้เห็นวิธีทำงานของฉัน และฉันก็รีวิว PR ของเขาเพื่อช่วยชี้ทิศทางให้ บางครั้งฉันก็ได้เรียนรู้จากพวกเขาเหมือนกัน
การจับบั๊กก็ใช่ แต่ไม่ควรเป็นกลไกหลัก และมีความสำคัญเป็นพิเศษกับบั๊กด้านความปลอดภัยและประสิทธิภาพที่จับด้วย automated test ได้ยาก
โดยเฉพาะในเรื่องmodularization และ decomposition ยิ่งเมื่อเข้าใจ PR ก้อนใหญ่ทั้งหมดแล้ว จะเริ่มมีโมเดลในหัวและมองออกว่ามันจะดูแลรักษาได้ กลายเป็นฝันร้ายเต็มรูปแบบในอนาคต หรืออยู่ตรงกลางระหว่างนั้น
สำหรับการรีวิวโค้ด ส่วนที่อาจสำคัญที่สุดคือการถ่ายทอดความรู้
ทีมเล็กของเราจะให้ทั้งทีมกดอนุมัติ PR ก่อน merge ถ้าไม่ใช่กรณีเร่งด่วน และผลคือทุกคนในทีมพอจะรู้สถานะปัจจุบันของ codebase โดยรวม จึงเลี่ยงเหตุเซอร์ไพรส์แบบ “ระบบทั้งก้อนที่ฉันพึ่งพาหายไปแล้ว” ซึ่งเคยเจอในบริษัทที่แยกเป็นไซโลมากกว่านี้
มันยังเป็นพื้นที่ให้ตั้งคำถามและเพิ่มความเข้าใจเกี่ยวกับวิธีการทำงานด้วย ถ้าเป็นทีมที่ทำงานกันดี นักพัฒนาทุกคนควรเข้าใจระบบโดยรวมในระดับหนึ่ง แม้กระทั่งส่วนที่ตัวเองไม่ได้ลงมือแตะโดยตรง
อีกหน้าที่สำคัญคือการตรวจสอบความรู้ในองค์กร ไม่นานมานี้ฉันปรับตารางนิดหน่อย แล้วเพื่อนร่วมงานก็บอกว่ามี microservice ที่ฉันไม่ได้นึกถึงซึ่งเขียนลงตารางนั้นอยู่และมันจะพัง ตอนนั้นฉันไม่รู้ด้วยซ้ำว่ามี microservice นั้นอยู่ หรือมันเข้าถึงตารางนั้น แต่การตรวจสอบนั้นช่วยกันปัญหาใหญ่กว่าและสถานการณ์ต้องมานั่ง cleanup ข้อมูลทีหลังได้
สุดท้ายทุกคนก็ยุ่งกับงานตัวเองและไม่อยากเป็นคนที่ขวาง PR สำคัญ เลยกดอนุมัติไปเฉย ๆ จนกลายเป็นสภาพที่ไม่มีใครรีวิวโค้ดจริง ๆ
สำหรับปัญหาแบบ “ระบบทั้งก้อนที่ฉันพึ่งพาหายไปแล้ว” ฉันคิดว่าautomated testสำคัญมาก เพราะมันควรจับปัญหาได้แม้คนที่พึ่งพาระบบนั้นจะไม่อยู่ตรงนั้น
ฉันยังสนับสนุนอย่างมากให้มี shared ownership กับทุกสิ่ง เป็นธรรมดาที่คนจะกลายเป็นเจ้าของโดยพฤตินัยของบางส่วนใน codebase โดยเฉพาะคอมโพเนนต์ที่ตัวเองสร้าง แต่สิ่งนี้นำไปสู่ไซโลและ bus factor ที่ต่ำ ไม่ควรมีโครงสร้างที่คนคนเดียวเป็นเจ้าของระบบหนึ่ง แล้วระบบนั้นไปพึ่งอีกคอมโพเนนต์หนึ่งต่อ
พอมีสิ่งให้อ่านมากเกินไป ก็ไม่มีใครตามทันได้ นั่นจึงเป็นเหตุผลที่ต้องมีการมอบหมาย ทำเอกสาร และจัดoverview session
ฉันคิดมาตลอดว่าการรีวิวโค้ดควรถูกมองเป็นด่านที่โค้ดซึ่งเคยเป็นของผู้เขียน เปลี่ยนไปเป็นทรัพย์สินของทีมหรือโปรเจกต์
โค้ดที่ฉันรีวิวไม่ใช่โค้ดของคุณ แต่เป็นโค้ดที่กำลังจะกลายเป็นโค้ดของพวกเรา แน่นอนว่าความสามารถในการดูแลรักษาเป็นองค์ประกอบใหญ่ในเรื่องนี้
ทีมเราเริ่มใช้ AI แล้ว ฉันเลยเปลี่ยนไปใช้วิธีง่าย ๆ ไม่เขียนคอมเมนต์แล้ว ตัดสินแบบไบนารีแค่ว่า “นี่มันบ้าระห่ำเกินรับได้ไหม หรือพอผ่านได้”
อย่างน้อยก็ช่วยประหยัดเวลาและสุขภาพจิตของฉัน
ทำแบบนี้มีแต่จะทำให้ทั้งผู้รีวิวและผู้เขียนขี้เกียจมากขึ้นเท่านั้น
จุดประสงค์ของการรีวิวโค้ดมีหลายมิติ: มันบำรุงรักษายากหรือไม่, อาจมีบั๊กหรือเปล่า, ทำให้เรียบง่ายและสะอาดกว่านี้ได้ไหม, สอดคล้องกับ code style ของโปรเจ็กต์หรือไม่, ช่วยให้คนอื่นเข้าใจโค้ดได้ไหม, ช่วย onboard สมาชิกทีมระดับจูเนียร์หรือไม่, และใช้ sanity check กับการตัดสินใจด้านสถาปัตยกรรมได้ไหม ทั้งหมดนี้ล้วนเกี่ยวข้อง
บทความเบา ๆ แบบนี้โดยมากใกล้เคียงกับการที่ ผู้รีวิวโค้ดขี้เกียจ หาเหตุผลเข้าข้างตัวเองมากกว่า
ควรดูว่าโค้ดบรรลุเป้าหมายเชิงฟังก์ชันตามที่อธิบายใน issue หรือ PR หรือไม่, มีโค้ดที่ไม่จำเป็นอย่าง debug output ที่ยังเหลืออยู่หรือ private API key หรือไม่, และมีข้อบกพร่องชัดเจนอย่าง memory leak, edge case ที่ยังไม่ถูกจัดการ, ช่องโหว่ความปลอดภัย, หรือการเรียก API แบบเก่าหรือไม่
ควรดูด้วยว่าสามารถทำให้อ่านเข้าใจง่ายขึ้นได้ไหม, ควรเพิ่มหรือลด abstraction หรือไม่, ชื่อตัวแปรและเมธอดดีพอหรือยัง, และควรใช้หรือหลีกเลี่ยงสไตล์แบบ functional มากกว่านี้หรือไม่
ควรตรวจด้วยว่าสอดคล้องกับ codebase หรือ style guide หรือไม่, มีการปรับปรุงประสิทธิภาพที่เห็นชัดหรือไม่ เช่น ใช้ hash set แทน list หรือใช้ lazy evaluation, และมีการทดสอบเพียงพอหรือไม่
ผมก็ไม่เห็นด้วยเต็มที่กับข้ออ้างที่ว่า ถ้าเป็นโค้ดที่เข้าใจไม่ได้ก็ไม่ควรผ่านเสมอไป บางโค้ดมันเข้าใจยากจริง ๆ และเป้าหมายคือทำให้มันถูกต้องตามการทำงานพร้อมทั้งเข้าใจได้ง่ายที่สุดเท่าที่ทำได้
แต่บทความนี้ก็ใกล้เคียงกับการล่อให้เถียงกันมากกว่า และคล้ายกับการพูดว่า “คนเราคิดว่ามื้อเย็นคือการกินอาหาร แต่จริง ๆ ไม่ใช่การกิน มันคือการเชื่อมโยงกับครอบครัวและเพื่อน” เป็นตรรกะแบบ ลดทอนเกินจริง ที่หละหลวมประเภทหนึ่งซึ่งใช้ได้ผลดีบน HN
ผมรู้สึกว่าการรีวิวและดีบักกินเวลามากกว่าการเขียนหรือผลิตโค้ดมาก และการแค่ “ภาวนาให้มันทำงาน” ก็ไม่เคยจบสวยเลย
คำพูดที่ว่า “ดูโค้ดเฉย ๆ โดยทั่วไปแล้วหาบั๊กไม่ได้” นั้นไม่จริงเลย ในแต่ละระดับของ abstraction ก็หาสิ่งเหล่านี้ได้เยอะพอสมควร และเราเรียกมันว่า code smell
ไม่ปิด file descriptor, coroutine ที่ไม่ได้ await, บล็อก try/catch ขนาดใหญ่ที่คืนค่าบางอย่างกลับมาโดยไม่ log error, การ cast type ผิด ฯลฯ ล้วนเข้าข่าย
ตามหลักทั่วไป ไม่ควรมอง type checker, compiler และ runtime ว่าเป็นแค่ด่านที่ต้องผ่านไปให้ได้ เราควรทำงานร่วมกับขั้นตอนเหล่านี้, ยอมรับว่ามันเป็นเครื่องมือที่มีคุณค่า, และไม่ควรทำงานสวนทางกับมัน
จะนิยามคำว่า “โดยทั่วไป” ให้มันจริงในเชิงเทคนิคก็อาจทำได้อยู่ แต่ถ้าทำแบบนั้นมันก็แทบไม่มีความหมายแล้ว
ถ้าจะพูดอ้างกว้าง ๆ เกี่ยวกับ code review การนิยามก่อนว่าหมายถึง code review แบบไหนเป็นเรื่องสำคัญ
ทุกวันนี้การรีวิว PR แบบ asynchronous บน GitHub พร้อมคอมเมนต์ inline กลายเป็นมาตรฐาน แต่การรีวิวไม่ได้มีแค่นั้น สมัยก่อนก็มีการรีวิวแบบเจอหน้ากัน ซึ่งกระบวนการคล้ายการสอบวิทยานิพนธ์หรือการนำเสนอในงานประชุมวิชาการ
งานเขียนที่บอกว่า code review เป็นแนวปฏิบัติด้านคุณภาพที่มีประโยชน์ หรือจริง ๆ แล้วเป็นหนึ่งในไม่กี่แนวปฏิบัติด้านคุณภาพที่มีประโยชน์ มักมาจากกระบวนการรีวิวที่มีโครงสร้างกว่าปัจจุบันมาก
โดยส่วนตัวแล้ว ผมรู้สึกว่าการรีวิว PR แบบ GitHub ก่อนยุค LLM มีไว้เพื่อทำให้รู้สึกว่ากระบวนการใช้ได้ หรือเพื่อทำเครื่องหมายเช็กบ็อกซ์ด้าน governance มากกว่า และใน ยุค LLM มันน่าจะยิ่งไม่คุ้มเมื่อเทียบต้นทุนกับผลลัพธ์จนคงค่อย ๆ หายไป
ถ้าจะใช้อุปมาเชิงเทคนิคแบบฝืน ๆ หน่อย การสื่อสารแบบข้อความ asynchronous มีการสูญเสียข้อมูลมากกว่าและ throughput ต่ำกว่าการพูด ในแง่ของปริมาณข้อมูลที่เข้ารหัสและส่งได้สำเร็จ ดังนั้นเวลาที่ต้องแลกเปลี่ยนข้อมูลจำนวนมาก การยอมจ่าย ต้นทุนของการซิงก์กัน ก็คุ้มค่า
มันใกล้เคียงกับวิศวกรรมแบบดั้งเดิมมากกว่า และทุกคนต้องมองซอฟต์แวร์ว่าเป็นสิ่งที่ถาวรกว่านี้
จริงอยู่ที่การรับประกันความสามารถในการบำรุงรักษาเป็นหนึ่งในประโยชน์ของ code review แต่การบอกว่านั่นคือจุดประสงค์เดียวถือเป็นข้ออ้างที่กล้ามาก
code review ยังเป็นเครื่องมือที่ช่วยให้ทีมรับรู้การเปลี่ยนแปลงของโค้ดและแบ่งปัน ความรับผิดชอบร่วมกัน ต่อ codebase ทั้งหมดด้วย
code review ไม่ใช่สิ่งเดียวที่มีความหมายตายตัว มันมีหลายเหตุผล ทั้งการแบ่งปันความรู้, การฟอกความรับผิดชอบ, คุณภาพโค้ด, การปฏิบัติตามข้อกำกับดูแล ฯลฯ และตามปกติแล้ว มันจะมีจุดมุ่งหมายแบบไหนก็ขึ้นอยู่กับ บริบทการใช้งาน
ถ้าผู้เขียนเป็นนักคณิตศาสตร์ คำว่า “ดูโค้ดเฉย ๆ โดยทั่วไปแล้วหาบั๊กไม่ได้” ก็คงไม่ได้หมายความว่าการหาบั๊กเป็นไปไม่ได้โดยสิ้นเชิง
มันใกล้เคียงกับการบอกว่าเราไม่สามารถหาบั๊กทั้งหมด หรือหาบั๊กเฉพาะตัวที่กำหนดไว้ ได้มากกว่า
และถ้าจะโยงกลับไปที่ประเด็นเรื่องการบำรุงรักษา เป้าหมายของการรีวิวอย่างหนึ่งก็คือทำให้โค้ดเรียบง่ายที่สุดเท่าที่ทำได้ เพื่อเพิ่มโอกาสที่จะ ดีบักผ่านการรีวิวได้ มันไม่ได้ป้องกันบั๊กในความหมายแบบสัมบูรณ์ แต่ช่วยเพิ่มความน่าจะเป็นได้
การตีความแบบแรกเกี่ยวข้องกันแต่ผิด ส่วนแบบที่สองจริงแต่ไม่เกี่ยวประเด็น
ผู้เขียนน่าจะตั้งใจจะบอกว่า “สำหรับบั๊กที่กำหนดมา การดูโค้ดเฉย ๆ โดยทั่วไปแล้วหาไม่เจอ” หรือก็คือ “สำหรับทุกบั๊ก B ไม่ได้หมายความว่าจะหา B เจอได้เสมอ” ซึ่งก็จริงแต่ก็ยังห่างจากประเด็นหลักอยู่ดี
เคยร่วมงานมาทั้งกับคนที่มักปฏิเสธข้อเสนอใน PR บ่อย ๆ และคนที่ยอมรับข้อเสนอ
คนที่ยอมรับข้อเสนอทำให้รู้สึกว่าเขาตั้งใจจะแบ่งปัน ownership กับฉันในระดับหนึ่ง ทั้งสองฝ่ายต่างช่วยกันดูแลและทำความเข้าใจโค้ด และรู้สึกเหมือนกำลังมองไปในทิศทางเดียวกัน
ในทางกลับกัน เมื่อเป็น PR ของคนที่มักปฏิเสธข้อเสนอ ก็จะรู้สึกอยากมีส่วนร่วมน้อยลง เพราะถ้ายังไงก็จะถูกปฏิเสธอยู่แล้ว จะเสียเวลามารีวิวละเอียดทำไม
thought,change,nit,fix,chatตัวอย่างเช่น
thoughtคือ “ต่อไปถ้า foo กลายเป็นสิ่งที่พบบ่อยขึ้น ค่อยมา refactor ตอนนั้นกัน”,changeคือ “นี่เป็น abstraction ที่รั่ว เลยอยากให้ model แบบ bar”,nitคือ “ชื่อนี้ยังไม่ค่อยตรงไปตรงมา ลองพิจารณา Baz หรือ Boo”,fixคือ “unit test นี้กำลังตรวจ field ผิด”, ส่วนchatคือ “นี่เป็นการตัดสินใจใหญ่ที่จะกำหนดรูปแบบของแนวทางแก้ปัญหาหมวดนี้ในอนาคต งั้นคุยกับทีมก่อน” ประมาณนี้คำนำหน้าบางแบบหมายถึงต้องหยุด PR ไว้จนกว่าจะมีการแก้ไข ส่วนบางแบบหมายถึงเป็นคอมเมนต์ที่รับหรือไม่รับก็ได้ ทำให้ผู้เขียนรู้ชัดว่าอะไรคือ “สิ่งที่ต้องเข้าใจให้ตรงกัน” และอะไรเป็นเพียง “ความชอบในการเขียน” หรือ “ข้อสังเกต”
แต่ถ้าทิ้ง
nitไว้แล้วอีกฝ่ายไม่เห็นด้วยและมองข้ามไป ก็ไม่ควรรู้สึกไม่พอใจ ถ้าคุณรู้สึกแรงขนาดนั้น มันก็ไม่ควรเป็นnitตั้งแต่แรก