Testing, Debugging, dan Kualitas Kode · 7/7

Code review, dua arah

Cara membuka pull request yang bisa direview orang, dan cara mereview tanpa jadi menyebalkan.

Baca 28 menit

Setelah pelajaran ini kamu bisa

Membuka pull request yang benar-benar bisa direview orang
Mereview kode orang lain secara berguna dan santun
Membedakan keberatan yang memblokir dari preferensi
Menerima review tanpa menganggapnya serangan pribadi

Code review adalah bagian pekerjaan yang berkaitan dengan manusia, dan di situlah reputasi seorang developer junior sebenarnya terbentuk. Keahlian teknisnya lebih kecil daripada keahlian sosialnya.

Membuka PR yang bisa direview

Faktor terbesar apakah PR-mu mendapat review yang baik adalah ukurannya. Perhatian reviewer turun tajam seiring jumlah baris, dan PR yang besar mendapat cap setuju alih-alih review.

Coba sendiri

Kenapa PR kecil mendapat review lebih baik. Angkanya ilustratif, tapi bentuknya sudah terdokumentasi baik.

// Defects present per 100 lines, and roughly what share a reviewer finds
// as the diff grows. Attention does not scale with size.
function reviewQuality(lines) {
  // A reviewer sustains careful attention for roughly 200-400 lines.
  const attention = lines <= 200 ? 0.9 : Math.max(0.15, 0.9 * (200 / lines));
  const defects = Math.round((lines / 100) * 1.5);
  return {
    lines: lines,
    defects: defects,
    found: Math.round(defects * attention),
    attention: Math.round(attention * 100) + "%",
  };
}

console.log("diff size".padEnd(12) + "defects".padStart(8) + "found".padStart(7) + "attention".padStart(11));
for (const lines of [50, 200, 500, 1000, 2000]) {
  const r = reviewQuality(lines);
  console.log(
    (r.lines + " lines").padEnd(12) + String(r.defects).padStart(8) +
    String(r.found).padStart(7) + r.attention.padStart(11)
  );
}

console.log("");
// The same total work, split up.
const asOnePr = reviewQuality(1000);
const asFivePrs = [200, 200, 200, 200, 200].map(reviewQuality);
const foundSplit = asFivePrs.reduce((sum, r) => sum + r.found, 0);

const totalDefects = asFivePrs.reduce((s, r) => s + r.defects, 0);
console.log("1000 lines as ONE pull request: " + asOnePr.found + " of " + asOnePr.defects + " defects found");
console.log("1000 lines as FIVE PRs:         " + foundSplit + " of " + totalDefects + " defects found");
console.log("");
console.log("Same code, same reviewer, same total lines. Splitting it up catches " +
  (foundSplit / asOnePr.found).toFixed(1) + "x more,");
console.log("which is why 'one PR, one purpose' is advice about effectiveness,");
console.log("not tidiness.");

Hasil

Tekan Jalankan untuk melihat hasilnya.

Ini jalan di browser kamu, di dalam sandbox. Apa pun yang kamu tulis di sini tidak bisa merusak situs.

Satu PR, satu tujuan. Perbaikan bug *dan* penggantian nama itu dua PR.
Tulis kenapa, bukan apa. Diff-nya sudah menunjukkan apa yang berubah. Deskripsinya menjelaskan kenapa itu perlu.
Sebutkan apa yang sudah kamu uji. "Sudah jalankan suite-nya; cek manual jalur timeout di 3G lambat."
Tandai keraguanmu sendiri. "Saya kurang yakin soal cache key di sini" akan mendatangkan bantuan alih-alih kritik kecil.
Review diff-mu sendiri dulu. Baca setiap baris seolah orang lain yang menulisnya. Kamu akan selalu menemukan sesuatu.
Jangan pernah mencampur reformat dengan perubahan. Diff-nya jadi tidak terbaca.

Mereview kode orang lain

Baca dengan urutan ini. Sebagian besar reviewer mulai dari baris 1 file pertama, dan itu tempat paling tidak berguna untuk memulai.

Deskripsinya. Ini untuk apa? Kalau kamu tidak bisa menebaknya, itu komentar pertamamu.
Test-nya. Mereka memberitahumu apa yang diyakini penulisnya seharusnya dilakukan kodenya.
Bentuk keseluruhannya. Apakah pendekatannya sudah tepat? Angkat sekarang — setelah detailnya sudah terlambat.
Kebenaran. Kasus tepi, jalur error, null yang akan kamu lupakan.
Keamanan. Apakah input-nya divalidasi? Apakah authorization diperiksa di tempat datanya disentuh?
Sisanya. Penamaan, struktur, duplikasi. Paling tidak penting, dan justru di situ sebagian besar komentar mendarat.

text

❌ "This is wrong."
✅ "blocking: this reads params.id without checking it is a number, so
    /venues/abc would reach the query. Worth a Number.isInteger guard?"

❌ "Why did you do it this way?"
✅ "question: what made you reach for a ref here rather than state? I might
    be missing a re-render problem."

❌ "Use map here."
✅ "nit: a .map() would read a little tighter, but the loop is fine too."

❌ "Did you even test this?"
✅ "I could not find a test for the empty-list case - is that covered
    somewhere I missed?"

Pola di setiap versi yang lebih baik: komentari kodenya, sebutkan akibatnya, dan sisakan ruang untuk kemungkinan kamu yang salah.

Menerima review

Ini soal kodenya. Tidak ada yang mereview PR-mu sedang menyerangmu. Butuh beberapa bulan sampai ini terasa benar, dan itu memang benar.
Jawab setiap komentar. Sekadar "sudah" atau "saya tidak setuju, karena…" pun cukup. Diam terbaca sebagai mengabaikan.
Tidak setuju itu boleh dan diharapkan. Kamu mungkin tahu sesuatu yang tidak diketahui reviewer-nya. Sebutkan apa itu.
Bertanya saat tidak paham. "Saya belum paham — bisa tunjukkan maksudnya?" adalah hal yang sangat normal untuk diucapkan.
Berterima kasih pada yang menemukan bug nyata. Mereka baru saja menyelamatkanmu dari sebuah insiden.
Review yang panjang itu pujian. Tidak ada yang menghabiskan satu jam untuk kode yang tidak ia pedulikan.

Tugas praktik

Review pekerjaanmu sendiri. Pilih file mana pun yang kamu buat saat mengerjakan jalur ini, buka, lalu tulis tiga komentar review untuknya — memakai label blocking/suggestion/nit/question. Lalu tindaklanjuti yang blocking. Mereview kodemu sendiri dalam kondisi "dingin" adalah hal paling mirip dengan mendapat review senior secara gratis.

Hasil yang diharapkan

Tiga komentar berlabel, dan satu perubahan yang benar-benar kamu lakukan.