ตรวจสอบรหัสแนวทางปฏิบัติที่ดีที่สุด

อินเทอร์เน็ตแสดงเนื้อหาที่หลากหลายเกี่ยวกับการรีวิวโค้ด: จากผลของการวิจารณ์โค้ดเกี่ยวกับวัฒนธรรมของ บริษัท ในบทวิจารณ์การรักษาความปลอดภัยที่เป็นทางการคู่มือที่สั้นกว่ารายการตรวจสอบที่ยาวขึ้นรีวิวที่มีมนุษยธรรมเหตุผลในการทำรีวิวโค้ดในตอนแรก การปฏิบัติ, สถิติเกี่ยวกับประสิทธิภาพการตรวจสอบรหัสสำหรับการจับข้อบกพร่องและตัวอย่างของการตรวจสอบรหัสผิดไป โอ้และแน่นอนว่ามีหนังสือด้วย เรื่องสั้นสั้น ๆ โพสต์บล็อกนี้นำเสนอรีวิวเกี่ยวกับโค้ดของ Palantir องค์กรที่มีวัฒนธรรมไม่เต็มใจที่จะวิจารณ์อย่างลึกซึ้งอาจต้องการศึกษาบทความที่ยอดเยี่ยมของ Karl E. Wiegers เกี่ยวกับรีวิว Humanizing Peer ก่อนลองทำตามคำแนะนำนี้

โพสต์นี้ถูกคัดลอกมาจากแนวทางปฏิบัติที่ดีที่สุดของห่วงโซ่เครื่องมือ Java Code Quality, Baseline และครอบคลุมหัวข้อต่อไปนี้:

  • ทำไมอะไรและเมื่อใดที่ต้องทำการตรวจสอบโค้ด
  • กำลังเตรียมรหัสสำหรับการตรวจสอบ
  • ดำเนินการตรวจสอบโค้ด
  • ตัวอย่างการตรวจสอบโค้ด

แรงจูงใจ

เราดำเนินการตรวจสอบโค้ด (CRs) เพื่อปรับปรุงคุณภาพของรหัสและรับประโยชน์จากผลกระทบเชิงบวกต่อทีมและวัฒนธรรมองค์กร ตัวอย่างเช่น:

  • ผู้กระทำการได้รับแรงบันดาลใจจากแนวคิดของผู้ตรวจสอบที่จะตรวจสอบคำขอเปลี่ยนแปลง: ผู้เดินทางมีแนวโน้มที่จะทำความสะอาดปลายที่หลวมรวมเข้าด้วยกันสิ่งที่ต้องทำและปรับปรุงความมุ่งมั่น การยอมรับความเชี่ยวชาญด้านการเขียนโปรแกรมผ่านเพื่อนเป็นแหล่งความภาคภูมิใจสำหรับโปรแกรมเมอร์จำนวนมาก
  • การแบ่งปันความรู้ช่วยทีมพัฒนาในหลายวิธี:
    - CR สื่อสารอย่างชัดเจนเพิ่ม / แก้ไข / ลบฟังก์ชันการทำงานให้กับสมาชิกในทีมที่สามารถสร้างงานที่ทำในภายหลัง
    - ผู้เดินทางอาจใช้เทคนิคหรืออัลกอริทึมที่ผู้ตรวจสอบสามารถเรียนรู้ได้ โดยทั่วไปการตรวจสอบโค้ดจะช่วยยกระดับคุณภาพทั่วทั้งองค์กร
    - ผู้ตรวจสอบอาจมีความรู้เกี่ยวกับเทคนิคการเขียนโปรแกรมหรือรหัสฐานที่สามารถช่วยปรับปรุงหรือรวมการเปลี่ยนแปลง ตัวอย่างเช่นบุคคลอื่นอาจทำงานพร้อมกันในคุณลักษณะหรือการแก้ไขที่คล้ายกัน
    - การมีปฏิสัมพันธ์เชิงบวกและการสื่อสารจะช่วยเสริมสร้างความผูกพันทางสังคมระหว่างสมาชิกในทีม
  • ความสอดคล้องในฐานรหัสทำให้การอ่านและทำความเข้าใจรหัสง่ายขึ้นช่วยป้องกันข้อผิดพลาดและอำนวยความสะดวกในการทำงานร่วมกันระหว่างสปีชีส์ของนักพัฒนาทั่วไปและผู้อพยพย้ายถิ่น
  • ความชัดเจนของรหัสเศษเล็กเศษน้อยนั้นยากที่จะตัดสินสำหรับผู้เขียนที่มีลูกเป็นสมองและง่ายต่อการตัดสินผู้ตรวจสอบที่ไม่มีบริบทเต็มรูปแบบ รหัสที่ชัดเจนนั้นสามารถนำมาใช้ซ้ำได้มากขึ้นปราศจากข้อผิดพลาดและเป็นหลักฐานในอนาคต
  • ข้อผิดพลาดของอุบัติเหตุ (เช่นตัวพิมพ์) รวมถึงข้อผิดพลาดทางโครงสร้าง (เช่นรหัสที่ไม่ทำงานตรรกะหรือข้อบกพร่องของอัลกอริทึมประสิทธิภาพหรือข้อกังวลด้านสถาปัตยกรรม) มักจะง่ายต่อการตรวจสอบผู้วิจารณ์ที่สำคัญด้วยมุมมองจากภายนอก การศึกษาพบว่าแม้แต่การทบทวนรหัสสั้น ๆ และไม่เป็นทางการก็มีผลกระทบอย่างมากต่อคุณภาพของรหัสและความถี่ข้อผิดพลาด
  • สภาพแวดล้อมด้านการปฏิบัติตามกฎระเบียบและข้อบังคับมักต้องการการตรวจสอบ CRs เป็นวิธีที่ดีในการหลีกเลี่ยงกับดักความปลอดภัยทั่วไป หากคุณลักษณะหรือสภาพแวดล้อมของคุณมีข้อกำหนดด้านความปลอดภัยที่สำคัญจะได้รับประโยชน์จาก (และอาจต้องมี) การตรวจสอบโดย curmudgeons ความปลอดภัยในพื้นที่ของคุณ (คำแนะนำของ OWASP เป็นตัวอย่างที่ดีของกระบวนการ)

รีวิวอะไร

ไม่มีคำตอบที่แท้จริงตลอดไปสำหรับคำถามนี้และทีมพัฒนาแต่ละทีมควรเห็นด้วยกับแนวทางของตัวเอง บางทีมชอบที่จะตรวจสอบการเปลี่ยนแปลงทุกอย่างที่รวมเข้าไปในสาขาหลักในขณะที่คนอื่น ๆ จะมีเกณฑ์ "เล็กน้อย" ซึ่งไม่จำเป็นต้องมีการทบทวน ข้อเสียคือการใช้เวลาอย่างมีประสิทธิภาพของวิศวกร (ทั้งผู้เขียนและผู้ตรวจสอบ) และการรักษาคุณภาพของรหัส ในบางสภาพแวดล้อมของกฎระเบียบอาจจำเป็นต้องมีการตรวจสอบโค้ดแม้มีการเปลี่ยนแปลงเล็กน้อย

การตรวจสอบรหัสไม่มีประเภท: การเป็นคนที่อาวุโสที่สุดในทีมไม่ได้หมายความว่าโค้ดของคุณไม่จำเป็นต้องมีการตรวจทาน แม้ว่าในกรณีที่หายากรหัสไม่สมบูรณ์การทบทวนให้โอกาสในการให้คำปรึกษาและการทำงานร่วมกันและอย่างน้อยที่สุดกระจายความเข้าใจของรหัสในฐานรหัส

ควรทบทวนเมื่อใด

การตรวจสอบโค้ดควรเกิดขึ้นหลังจากการตรวจสอบอัตโนมัติ (การทดสอบรูปแบบและ CI อื่น ๆ ) เสร็จสมบูรณ์แล้ว แต่ก่อนที่รหัสจะรวมเข้ากับสาขาหลักของพื้นที่เก็บข้อมูล โดยทั่วไปเราจะไม่ทำการตรวจสอบรหัสอย่างเป็นทางการของการเปลี่ยนแปลงโดยรวมตั้งแต่รุ่นล่าสุด

สำหรับการเปลี่ยนแปลงที่ซับซ้อนที่ควรรวมเข้าไปในสาขาการฉีดเป็นหน่วยเดียว แต่มีขนาดใหญ่เกินกว่าที่จะใส่ลงใน CR ที่สมเหตุสมผลหนึ่งให้พิจารณาโมเดล CR แบบเรียงซ้อน: สร้างคุณลักษณะสาขาหลัก / คุณสมบัติใหญ่และจำนวนสาขารอง (เช่นคุณสมบัติ / big-feature-api, คุณสมบัติ / การทดสอบคุณสมบัติขนาดใหญ่ ฯลฯ ) ที่แต่ละแค็ปซูลย่อยของฟังก์ชั่นและที่ได้รับการตรวจสอบรหัสเป็นรายบุคคลกับฟีเจอร์ / สาขาคุณสมบัติขนาดใหญ่ เมื่อสาขาย่อยทั้งหมดถูกรวมเข้ากับฟีเจอร์ / ฟีเจอร์ใหญ่ให้สร้าง CR สำหรับการรวมสาขาย่อยเข้ากับสาขาหลัก

กำลังเตรียมรหัสสำหรับการตรวจสอบ

เป็นความรับผิดชอบของผู้เขียนในการส่ง CR ที่ง่ายต่อการตรวจสอบเพื่อไม่ให้เสียเวลาและแรงจูงใจของผู้ตรวจสอบ:

  • ขอบเขตและขนาด การเปลี่ยนแปลงควรมีขอบเขตแคบ ๆ ที่กำหนดไว้อย่างดีในตัวเองซึ่งครอบคลุมอย่างละเอียดถี่ถ้วน ตัวอย่างเช่นการเปลี่ยนแปลงอาจใช้คุณสมบัติใหม่หรือแก้ไขข้อบกพร่อง การเปลี่ยนแปลงที่สั้นกว่าเป็นที่ต้องการมากกว่าการเปลี่ยนแปลงที่ยาวกว่า หาก CR ทำการเปลี่ยนแปลงที่สำคัญในไฟล์มากกว่า ~ 5 ไฟล์หรือใช้เวลานานกว่า 1-2 วันในการเขียนหรือใช้เวลามากกว่า 20 นาทีในการตรวจสอบให้พิจารณาแบ่งไฟล์ออกเป็น CR ที่มีอยู่ในตัวเองหลายตัว ตัวอย่างเช่นผู้พัฒนาสามารถส่งการเปลี่ยนแปลงหนึ่งรายการที่กำหนด API สำหรับคุณลักษณะใหม่ในแง่ของส่วนต่อประสานและเอกสารประกอบและการเปลี่ยนแปลงครั้งที่สองที่เพิ่มการใช้งานสำหรับส่วนต่อประสานเหล่านั้น
  • ส่งเฉพาะ CRS ที่สมบูรณ์ตรวจสอบด้วยตนเอง (โดยต่าง) และทดสอบด้วยตนเอง เพื่อประหยัดเวลาผู้ตรวจสอบให้ทดสอบการเปลี่ยนแปลงที่ส่งมา (เช่นเรียกใช้ชุดทดสอบ) และตรวจสอบให้แน่ใจว่าพวกเขาผ่านการสร้างทั้งหมดรวมถึงการทดสอบทั้งหมดและการตรวจสอบคุณภาพของรหัสทั้งในเครื่องและบนเซิร์ฟเวอร์ CI ก่อนกำหนดผู้ตรวจสอบ
  • การเปลี่ยนการปรับโครงสร้างใหม่ไม่ควรเปลี่ยนพฤติกรรม ในทางกลับกันการเปลี่ยนแปลงที่เปลี่ยนแปลงพฤติกรรมควรหลีกเลี่ยงการเปลี่ยนโครงสร้างและการจัดรูปแบบโค้ด มีหลายเหตุผลที่ดีสำหรับสิ่งนี้:
    - การเปลี่ยนการปรับโครงสร้างใหม่มักจะสัมผัสหลายบรรทัดและไฟล์และจะได้รับการตรวจสอบด้วยความสนใจน้อยลง การเปลี่ยนแปลงพฤติกรรมที่ไม่ตั้งใจสามารถรั่วไหลลงในฐานรหัสโดยไม่มีใครสังเกตเห็น
    - การเปลี่ยนแปลงการปรับโครงสร้างซ้ำจำนวนมากจะทำลายการเก็บเชอร์รี่การรีบูตและการควบคุมแหล่งเวทมนตร์อื่น ๆ มันเป็นเรื่องยากมากที่จะยกเลิกการเปลี่ยนแปลงพฤติกรรมที่ถูกนำมาใช้เป็นส่วนหนึ่งของการปรับโครงสร้างที่เก็บข้อมูลทั่วทั้งคอมมิชชัน
    - ควรใช้เวลาในการตรวจสอบจากคนที่มีค่าใช้จ่ายในตรรกะของโปรแกรมมากกว่าการใช้รูปแบบ, ไวยากรณ์หรือการจัดรูปแบบการอภิปราย เราชอบที่จะใช้เครื่องมืออัตโนมัติเช่น Checkstyle, TSLint, Baseline, Prettier เป็นต้น

ส่งข้อความ

ต่อไปนี้เป็นตัวอย่างของข้อความยืนยันที่ดีตามมาตรฐานที่ยกมาอย่างกว้างขวาง:

สรุปตัวพิมพ์ใหญ่, สั้น (80 ตัวอักษรหรือน้อยกว่า)
ข้อความอธิบายรายละเอียดเพิ่มเติมหากจำเป็น ห่อไว้ประมาณ 120 ตัวอักษรหรือมากกว่านั้น ในบริบทบางอย่างแรก
บรรทัดถือว่าเป็นหัวเรื่องของอีเมลและส่วนที่เหลือของข้อความเป็นเนื้อหา บรรทัดว่างคั่น
บทสรุปจากเนื้อหานั้นมีความสำคัญ (เว้นแต่คุณจะละเว้นเนื้อหาทั้งหมด) เครื่องมือเช่น rebase อาจสับสนถ้าคุณเรียกใช้
ทั้งสองเข้าด้วยกัน
เขียนข้อความการส่งของคุณในสิ่งที่จำเป็น: "แก้ไขข้อบกพร่อง" และไม่ใช่ "แก้ไขข้อผิดพลาด" หรือ "แก้ไขข้อบกพร่อง" การประชุมนี้ตรงกับ
ขึ้นกับการส่งข้อความที่สร้างโดยคำสั่งเช่น git merge และ git revert
ย่อหน้าเพิ่มเติมมาหลังจากบรรทัดว่าง
- สัญลักษณ์แสดงหัวข้อย่อยก็โอเคเช่นกัน

พยายามอธิบายทั้งการกระทำการเปลี่ยนแปลงและวิธีการ:

> BAD อย่าทำอย่างนี้
ทำการรวบรวมอีกครั้ง
> ดี
เพิ่มการพึ่งพา jcsv เพื่อแก้ไขการรวบรวม IntelliJ

การหาผู้ตรวจสอบ

มันเป็นเรื่องธรรมดาสำหรับผู้เดินทางเพื่อเสนอผู้ตรวจสอบหนึ่งหรือสองคนที่คุ้นเคยกับรหัสฐาน บ่อยครั้งที่หนึ่งในผู้ตรวจสอบคือหัวหน้าโครงการหรือวิศวกรอาวุโส เจ้าของโครงการควรพิจารณาสมัครเป็นสมาชิกโครงการของพวกเขาเพื่อรับการแจ้งเตือนของ CR ใหม่ การตรวจสอบรหัสระหว่างสามฝ่ายนั้นมักจะไม่ก่อผลหรือแม้แต่เป็นการต่อต้านเนื่องจากผู้ตรวจสอบที่แตกต่างกันอาจเสนอการเปลี่ยนแปลงที่ขัดแย้ง สิ่งนี้อาจบ่งบอกถึงความขัดแย้งขั้นพื้นฐานในการใช้งานที่ถูกต้องและควรได้รับการแก้ไขนอกการตรวจสอบโค้ดในฟอรัมที่มีแบนด์วิดท์สูงกว่าเช่นในบุคคลหรือในการประชุมทางวิดีโอกับทุกฝ่ายที่เกี่ยวข้อง

ดำเนินการตรวจสอบโค้ด

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

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

ในฐานะผู้ตรวจสอบคุณมีหน้าที่รับผิดชอบในการบังคับใช้มาตรฐานการเข้ารหัสและปรับปรุงคุณภาพของบาร์ การทบทวนรหัสเป็นศิลปะมากกว่าวิทยาศาสตร์ วิธีเดียวที่จะเรียนรู้มันคือการทำมัน; ผู้ตรวจสอบที่มีประสบการณ์ควรพิจารณานำผู้ตรวจสอบที่มีประสบการณ์น้อยกว่าคนอื่นมาเปลี่ยนแปลงและให้พวกเขาทำการตรวจสอบก่อน สมมติว่าผู้เขียนได้ปฏิบัติตามแนวทางข้างต้น (โดยเฉพาะอย่างยิ่งเกี่ยวกับการตรวจสอบตนเองและรับรองการรันโค้ด) นี่คือรายการสิ่งที่ผู้ตรวจสอบควรให้ความสนใจในการตรวจสอบรหัส:

วัตถุประสงค์

  • รหัสนี้บรรลุวัตถุประสงค์ของผู้เขียนหรือไม่ การเปลี่ยนแปลงทุกอย่างควรมีเหตุผลที่เฉพาะเจาะจง (คุณสมบัติใหม่ตัวสร้างข้อผิดพลาด ฯลฯ ) รหัสที่ส่งมาบรรลุวัตถุประสงค์นี้จริงหรือไม่?
  • ถามคำถาม. ฟังก์ชั่นและชั้นเรียนควรมีอยู่ด้วยเหตุผล เมื่อเหตุผลไม่ชัดเจนต่อผู้ตรวจสอบนี่อาจเป็นข้อบ่งชี้ว่าต้องเขียนรหัสใหม่หรือสนับสนุนพร้อมกับความคิดเห็นหรือการทดสอบ

การดำเนินงาน

  • คิดว่าคุณจะแก้ปัญหาอย่างไร ถ้ามันต่างกันทำไมล่ะ รหัสของคุณรองรับกรณี (ขอบ) มากขึ้นหรือไม่ มันสั้นลง / ง่ายขึ้น / สะอาดขึ้น / เร็วขึ้น / ปลอดภัยขึ้นกว่าเดิมอีกไหม? มีรูปแบบพื้นฐานบางอย่างที่คุณเห็นซึ่งไม่ได้ถูกจับด้วยรหัสปัจจุบันหรือไม่?
  • คุณเห็นศักยภาพของ abstractions ที่เป็นประโยชน์หรือไม่? รหัสที่ทำซ้ำบางส่วนมักจะระบุว่าสามารถแยกส่วนการทำงานที่เป็นนามธรรมหรือทั่วไปได้มากกว่าและนำมาใช้ซ้ำในบริบทที่แตกต่างกัน
  • คิดว่าเป็นปฏิปักษ์ แต่จะดีกับมัน พยายามที่จะ "จับ" ผู้เขียนใช้ทางลัดหรือกรณีที่ขาดหายไปด้วยการกำหนดค่าที่เป็นปัญหา / ข้อมูลอินพุตที่ทำลายรหัสของพวกเขา
  • คิดถึงไลบรารีหรือรหัสผลิตภัณฑ์ที่มีอยู่ เมื่อมีคนนำการทำงานที่มีอยู่กลับมาใช้บ่อยครั้งมากกว่าเพราะมันไม่รู้ว่ามีอยู่แล้ว บางครั้งรหัสหรือฟังก์ชั่นการทำซ้ำตามวัตถุประสงค์เช่นเพื่อหลีกเลี่ยงการอ้างอิง ในกรณีเช่นนี้ความคิดเห็นรหัสสามารถชี้แจงเจตนา ฟังก์ชั่นที่นำเสนอมีให้โดยห้องสมุดที่มีอยู่แล้วหรือไม่?
  • การเปลี่ยนแปลงเป็นไปตามรูปแบบมาตรฐานหรือไม่ รหัสฐานที่จัดตั้งขึ้นมักจะแสดงรูปแบบรอบ ๆ แบบแผนการตั้งชื่อการสลายตัวของตรรกะโปรแกรมคำจำกัดความชนิดข้อมูล ฯลฯ โดยทั่วไปแล้วการเปลี่ยนแปลงที่เกิดขึ้นนั้นเป็นสิ่งที่พึงปรารถนา
  • การเปลี่ยนแปลงเพิ่มการคอมไพล์เวลาหรือรันไทม์ (โดยเฉพาะระหว่างโครงการย่อย) หรือไม่ เราต้องการให้ผลิตภัณฑ์ของเราอยู่คู่กันอย่างอิสระโดยมีการพึ่งพาน้อยที่สุดเท่าที่จะทำได้ การเปลี่ยนแปลงการพึ่งพาและระบบการสร้างควรได้รับการพิจารณาอย่างหนัก

ความชัดเจนและสไตล์

  • คิดถึงประสบการณ์การอ่านของคุณ คุณเข้าใจแนวคิดในเวลาที่เหมาะสมหรือไม่? flow sane และชื่อตัวแปรและเมธอดติดตามได้ง่ายหรือไม่? คุณสามารถติดตามหลายไฟล์หรือฟังก์ชั่นได้หรือไม่? คุณถูกเลื่อนออกไปโดยการตั้งชื่อที่ไม่สอดคล้องกันหรือไม่?
  • รหัสเป็นไปตามแนวทางการเข้ารหัสและลักษณะของรหัสหรือไม่ รหัสสอดคล้องกับโครงการในแง่ของรูปแบบการประชุม API ฯลฯ หรือไม่ ดังที่กล่าวไว้ข้างต้นเราชอบที่จะยุติการอภิปรายสไตล์ด้วยเครื่องมืออัตโนมัติ
  • รหัสนี้มีสิ่งที่ต้องทำ? สิ่งที่ต้องทำก็พอกพูนในโค้ดและค้างอยู่ตลอดเวลา ให้ผู้เขียนส่งตั๋วเกี่ยวกับปัญหา GitHub หรือ JIRA และแนบหมายเลขปัญหาไว้ในสิ่งที่ต้องทำ การเปลี่ยนแปลงรหัสที่เสนอไม่ควรมีรหัสความคิดเห็นออก

การบำรุงรักษา

  • อ่านการทดสอบ หากไม่มีการทดสอบและควรมีให้ผู้เขียนเขียน คุณลักษณะที่ไม่สามารถพิสูจน์ได้อย่างแท้จริงนั้นหายากในขณะที่การใช้งานคุณสมบัติที่ไม่ผ่านการทดสอบนั้นเป็นเรื่องปกติ ตรวจสอบการทดสอบตัวเอง: พวกเขาครอบคลุมกรณีที่น่าสนใจ? พวกเขาอ่านหรือไม่ การครอบคลุมการทดสอบโดยรวมลดลงหรือไม่ นึกถึงวิธีที่รหัสนี้อาจแตก มาตรฐานสไตล์สำหรับการทดสอบมักจะแตกต่างจากรหัสหลัก แต่ก็ยังสำคัญ
  • CR นี้แนะนำความเสี่ยงของการทำลายรหัสการทดสอบการจัดเตรียมสแต็กหรือการทดสอบการรวมระบบหรือไม่? สิ่งเหล่านี้มักจะไม่ได้รับการตรวจสอบเนื่องจากเป็นส่วนหนึ่งของการตรวจสอบล่วงหน้า / รวม แต่การที่พวกเขาลงไปนั้นเป็นสิ่งที่เจ็บปวดสำหรับทุกคน สิ่งที่ต้องมองหาคือ: การลบยูทิลิตี้การทดสอบหรือโหมดการเปลี่ยนแปลงในการกำหนดค่าและการเปลี่ยนแปลงในโครงร่าง / โครงสร้างสิ่งประดิษฐ์
  • การเปลี่ยนแปลงนี้ทำลายความเข้ากันได้ย้อนหลังหรือไม่ ถ้าเป็นเช่นนั้นจะเป็นการรวมการเปลี่ยนแปลง ณ จุดนี้หรือควรผลักเข้าไปในรุ่นที่ใหม่กว่าหรือไม่? ตัวแบ่งอาจรวมถึงการเปลี่ยนแปลงฐานข้อมูลหรือสคีมาการเปลี่ยนแปลง API สาธารณะการเปลี่ยนแปลงเวิร์กโฟลว์ของผู้ใช้ ฯลฯ
  • รหัสนี้จำเป็นต้องมีการทดสอบการรวม? บางครั้งรหัสไม่สามารถทดสอบได้อย่างเพียงพอกับการทดสอบหน่วยโดยเฉพาะอย่างยิ่งโดยเฉพาะอย่างยิ่งถ้ารหัสโต้ตอบกับระบบภายนอกหรือการกำหนดค่า
  • แสดงความคิดเห็นเกี่ยวกับเอกสารระดับรหัสความคิดเห็นและการส่งข้อความ ความคิดเห็นที่ซ้ำซ้อนจะทำให้รหัสยุ่งเหยิง สิ่งนี้ไม่สามารถใช้ได้เสมอไป แต่การแสดงความคิดเห็นคุณภาพและการส่งข้อความจะจ่ายให้เอง (ลองนึกถึงเวลาที่คุณเห็นข้อความที่ยอดเยี่ยมหรือแย่จริง ๆ ยอมรับหรือแสดงความคิดเห็น)
  • มีการปรับปรุงเอกสารภายนอกหรือไม่ หากโครงการของคุณดูแล README, CHANGELOG หรือเอกสารอื่น ๆ มันถูกอัพเดทเพื่อสะท้อนการเปลี่ยนแปลงหรือไม่? เอกสารที่ล้าสมัยอาจทำให้เกิดความสับสนมากกว่าไม่มีและจะมีค่าใช้จ่ายเพิ่มเติมในการแก้ไขในอนาคตกว่าจะอัปเดตทันที

อย่าลืมชมรหัสที่กระชับ / อ่านได้ / มีประสิทธิภาพ / สวยงาม ในทางกลับกันการปฏิเสธหรือไม่ยอมรับ CR ไม่ได้หยาบคาย หากการเปลี่ยนแปลงนั้นซ้ำซ้อนหรือไม่เกี่ยวข้องให้ปฏิเสธด้วยคำอธิบาย หากคุณพิจารณาว่าไม่สามารถยอมรับได้เนื่องจากข้อบกพร่องร้ายแรงอย่างน้อยหนึ่งข้อให้ปฏิเสธอีกครั้งพร้อมคำอธิบาย บางครั้งผลลัพธ์ที่ถูกต้องของ CR คือ "มาทำแบบนี้กันเถอะ" หรือแม้แต่ "อย่าทำแบบนี้เลย"

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

ความปลอดภัย

ตรวจสอบว่าปลายทาง API ดำเนินการอนุญาตที่เหมาะสมและรับรองความถูกต้องสอดคล้องกับส่วนที่เหลือของรหัสฐาน ตรวจสอบจุดอ่อนทั่วไปอื่น ๆ เช่นการกำหนดค่าอ่อนการป้อนข้อมูลผู้ใช้ที่เป็นอันตรายเหตุการณ์บันทึกที่ขาดหายไป ฯลฯ หากมีข้อสงสัยให้อ้างอิง CR ไปยังผู้เชี่ยวชาญด้านความปลอดภัยของแอปพลิเคชัน

ความคิดเห็น: กระชับเป็นมิตรดำเนินการได้

ความคิดเห็นควรกระชับและเขียนในภาษาที่เป็นกลาง วิจารณ์รหัสไม่ใช่ผู้เขียน เมื่อบางสิ่งไม่ชัดเจนให้ถามเพื่อความชัดเจนมากกว่าที่จะเข้าใจ หลีกเลี่ยงคำสรรพนามโดยเฉพาะอย่างยิ่งร่วมกับการประเมินผล: "รหัสของฉันทำงานก่อนการเปลี่ยนแปลงของคุณ", "วิธีการของคุณมีข้อบกพร่อง" ฯลฯ หลีกเลี่ยงการตัดสินอย่างสมบูรณ์: "สิ่งนี้ไม่สามารถทำงานได้", "ผลลัพธ์ไม่ถูกต้องเสมอ"

ลองแยกความแตกต่างระหว่างข้อเสนอแนะ (เช่น“ ข้อเสนอแนะ: วิธีการแยกเพื่อปรับปรุงความชัดเจน”) การเปลี่ยนแปลงที่ต้องการ (เช่น“ เพิ่ม @Override”) และจุดที่ต้องมีการสนทนาหรือชี้แจง (เช่น“ นี่เป็นพฤติกรรมที่ถูกต้องจริงหรือไม่? ดังนั้นโปรดเพิ่มความคิดเห็นเพื่ออธิบายเหตุผล”) พิจารณาให้การเชื่อมโยงหรือตัวชี้ไปที่คำอธิบายในเชิงลึกของปัญหา

เมื่อคุณตรวจสอบโค้ดเสร็จแล้วให้ระบุว่าคุณคาดหวังว่าผู้เขียนจะตอบกลับความคิดเห็นของคุณหรือไม่และคุณต้องการทบทวน CR อีกครั้งหลังจากที่มีการใช้งานการเปลี่ยนแปลงหรือไม่ (เช่น“ รู้สึกอิสระที่จะผสานหลังจากตอบ ต่อข้อเสนอแนะเล็ก ๆ น้อย ๆ ” กับ“ โปรดพิจารณาข้อเสนอแนะของฉันและแจ้งให้ฉันทราบเมื่อฉันสามารถดูอีกครั้ง”)

ตอบสนองต่อความคิดเห็น

ส่วนหนึ่งของวัตถุประสงค์ของการตรวจสอบโค้ดคือปรับปรุงคำขอเปลี่ยนแปลงของผู้เขียน ดังนั้นอย่าโกรธเคืองตามคำแนะนำของผู้ตรวจทานและพิจารณาอย่างจริงจังแม้ว่าคุณจะไม่เห็นด้วยก็ตาม ตอบทุกความคิดเห็นแม้ว่าจะเป็นเพียง“ ACK” หรือ“ เสร็จสิ้น” อธิบายว่าทำไมคุณตัดสินใจบางอย่างทำไมถึงมีฟังก์ชั่นบางอย่างอยู่ ฯลฯ หากคุณไม่สามารถตกลงกับผู้ตรวจสอบได้ การสื่อสารเวลาหรือแสวงหาความเห็นจากภายนอก

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

ทีมที่แตกต่างกันมีนโยบายการรวมที่แตกต่างกัน: บางทีมอนุญาตเฉพาะเจ้าของโครงการที่จะผสานในขณะที่ทีมอื่น ๆ อนุญาตให้ผู้มีส่วนร่วมผสานหลังจากการตรวจสอบโค้ดที่เป็นบวก

บทวิจารณ์รหัสส่วนตัว

สำหรับความคิดเห็นส่วนใหญ่ของรหัสเครื่องมือที่ใช้แบบอะซิงโครนัสเช่นรีวิวได้ Gerrit หรือ GitHub เป็นตัวเลือกที่ยอดเยี่ยม การเปลี่ยนแปลงที่ซับซ้อนหรือการตรวจสอบระหว่างฝ่ายที่มีความเชี่ยวชาญหรือประสบการณ์ที่แตกต่างกันมากจะมีประสิทธิภาพมากขึ้นเมื่อดำเนินการด้วยตนเองไม่ว่าจะเป็นหน้าจอเดียวกันหรือโปรเจคเตอร์หรือจากระยะไกลผ่าน VTC หรือเครื่องมือแชร์หน้าจอ

ตัวอย่าง

ในตัวอย่างต่อไปนี้ข้อคิดเห็นการตรวจทานที่แนะนำจะถูกระบุโดย // R: ... ข้อคิดเห็นในบล็อคโค้ด

การตั้งชื่อไม่สอดคล้องกัน

คลาส MyClass {
  ส่วนตัว int countTotalPageVisits; // R: ตัวแปรชื่ออย่างสม่ำเสมอ
  ไพรเวต int uniqueUsersCount;
}

ลายเซ็นวิธีการที่ไม่สอดคล้องกัน

อินเตอร์เฟซ MyInterface {
  / ** คืน {@link # ว่าง} ถ้าไม่สามารถแยกได้ * /
  ตัวเลือก  แยกสาธารณะ String (String s);
  / ** คืนค่า null หาก {@code s} ไม่สามารถเขียนใหม่ได้ * /
  // R: ควรประสานค่าส่งคืน: ใช้ตัวเลือก <> ที่นี่ด้วย
  String สาธารณะ rewriteString (String s);
}

การใช้ห้องสมุด

// R: ลบและแทนที่ด้วย MapJoiner ของ Guava
String joinAndConcatenate (แผนที่  แผนที่, String keyValueSeparator, String KeySeparator);

รสนิยมส่วนตัว

int จำนวนวัน; // R: nit: ฉันมักจะชอบ numFoo มากกว่า fooCount; ขึ้นอยู่กับคุณ แต่เราควรทำให้มันสอดคล้องกันในโครงการนี้

คลั่ง

// R: นี่เป็นการคำนวณ numIterations + 1 การทำซ้ำ, นั่นเป็นการจงใจหรือไม่?
// ถ้าใช่ลองเปลี่ยนความหมายของตัวเลข?
สำหรับ (int i = 0; i <= numIterations; ++ i) {
  ...
}

ความกังวลทางสถาปัตยกรรม

otherService.call (); // R: ฉันคิดว่าเราควรหลีกเลี่ยงการพึ่งพาบริการอื่น ๆ เราสามารถพูดคุยเรื่องนี้ด้วยตนเองได้ไหม?

ผู้เขียน

Robert F (GitHub / Twitter)