Code Review Checklist: What I Look for After 10 Years of PRs \u2014 COD-AI.com

Tiga tahun yang lalu, saya menyetujui sebuah permintaan tarik yang mengakibatkan perusahaan saya kehilangan $47.000 dalam pendapatan hanya dalam satu akhir pekan. Kodenya terlihat baik. Uji coba berhasil. Logika tampak benar. Tapi saya melewatkan sesuatu yang halus dalam cara kami menangani konversi zona waktu untuk sistem penagihan langganan kami, dan pelanggan di wilayah tertentu dikenakan biaya dua kali.

Insiden itu mengubah cara saya meninjau kode selamanya. Saya Sarah Chen, dan saya telah menjadi manajer teknik senior di tiga perusahaan SaaS yang berbeda selama satu dekade terakhir, meninjau rata-rata 15-20 permintaan tarik per minggu. Itu sekitar 8.000 PR dalam karir saya. Saya telah melihat kode brilian yang mengirimkan bug, dan kode berantakan yang berjalan tanpa cacat di produksi selama bertahun-tahun. Saya telah belajar bahwa tinjauan kode bukan tentang menemukan kesalahan sintaksis—linter Anda melakukan itu. Ini tentang menangkap masalah tak terlihat yang hanya bisa diungkapkan oleh pengalaman.

Artikel ini adalah daftar periksa yang saya harap saya miliki saat saya mulai. Ini tidak komprehensif—tidak ada daftar periksa yang bisa—tapi ini merepresentasikan pola yang saya pelajari untuk dikenali, pertanyaan yang saya latih untuk diajukan, dan model mental yang telah menyelamatkan tim saya dari banyak insiden produksi.

30 Detik Pertama: Apa yang Dikatakan Deskripsi PR

Sebelum saya melihat satu baris kode, saya menghabiskan 30 detik untuk membaca deskripsi PR. Ini mungkin terlihat terbalik, tetapi saya telah menemukan bahwa 60% dari PR yang bermasalah mengungkapkan diri mereka segera melalui komunikasi yang buruk. Deskripsi PR yang baik menjawab tiga pertanyaan: apa yang berubah, mengapa itu berubah, dan apa yang bisa salah.

Ketika saya melihat deskripsi yang mengatakan "memperbaiki bug" atau "memperbarui komponen," saya segera tahu bahwa tinjauan ini akan memakan waktu lebih lama. Penulis belum memikirkan implikasi dari perubahan mereka, yang berarti saya harus melakukan pekerjaan itu untuk mereka. Sebaliknya, ketika saya melihat deskripsi yang mengatakan "Mengubah otentikasi pengguna untuk menggunakan token JWT alih-alih cookie sesi. Ini mengurangi beban database sebesar 40% selama jam sibuk tetapi memerlukan klien untuk menangani penyegaran token. Risiko: versi aplikasi seluler yang ada (< 2.3) akan perlu otentikasi ulang," saya tahu penulis telah melakukan pekerjaan rumah mereka.

Saya mencari metrik spesifik dalam deskripsi. Bukan perbaikan yang samar, tetapi angka yang sebenarnya. "Meningkatkan kinerja" tidak berarti apa-apa. "Mengurangi waktu respons API dari 340ms menjadi 180ms untuk titik akhir profil pengguna di bawah 1000 permintaan bersamaan" memberi tahu saya bahwa penulis mengukur pekerjaan mereka dan memahami dampaknya. Dalam pengalaman saya, pengembang yang menyertakan metrik dalam deskripsi PR mereka menulis kode yang lebih dipikirkan secara keseluruhan.

Deskripsi juga harus menghubungkan konteks yang relevan—tiket, dokumen desain, utas Slack di mana pendekatannya dibahas. Jika saya meninjau PR secara terpisah, tanpa memahami konteks yang lebih luas, saya akan melewatkan banyak hal. Saya pernah menyetujui "refactoring sederhana" yang tanpa sengaja merusak integrasi kritis karena saya tidak tahu tentang ketergantungan yang tidak didokumentasikan di mana pun kecuali di utas email tiga bulan yang lalu.

Bendera merah dalam deskripsi PR termasuk: tidak ada deskripsi sama sekali, deskripsi yang lebih panjang daripada perubahan kode itu sendiri (biasanya menunjukkan rekayasa berlebihan), deskripsi yang mengatakan "WIP" atau "draf" tetapi PR ditandai sebagai siap untuk ditinjau, dan deskripsi yang menyertakan frasa seperti "tidak yakin apakah ini pendekatan yang benar" (lalu mengapa kamu meminta saya untuk meninjaunya?).

Ukuran Itu Penting: Aturan 400 Baris

Saya punya aturan keras: saya tidak menjelaskan PR yang lebih besar dari 400 baris perubahan kode sebenarnya (tidak termasuk kode yang dihasilkan, file package-lock, dan fixture tes). Ini bukan sembarangan. Penelitian dari Cisco dan SmartBear menunjukkan bahwa efektivitas tinjauan kode turun drastis setelah 400 baris, dan pengalaman saya sendiri mengonfirmasi ini. Setelah meninjau sekitar 200 baris kode, otak saya mulai mengabaikan detail. Pada 400 baris, saya pada dasarnya hanya sedang membaca sekilas.

"Tinjauan kode bukan tentang menemukan kesalahan sintaksis—linter Anda melakukan itu. Ini tentang menangkap masalah tak terlihat yang hanya bisa diungkapkan oleh pengalaman."

Ketika seseorang mengajukan PR sepanjang 1.200 baris, saya meminta mereka untuk memecahnya. Saya tidak peduli jika itu "semua terkait." PR besar adalah tempat bug bersembunyi. Saya telah melihat kerawanan keamanan kritis meluncur dalam PR refactoring besar karena para peninjau menjadi lelah dan berhenti memperhatikan sekitar baris 600. Kerawanan itu berada di baris 847.

Ada pengecualian, tentu saja. Terkadang Anda perlu memindahkan file, atau memperbarui kode yang dihasilkan, atau melakukan perubahan besar untuk memenuhi standar linter baru. Dalam kasus tersebut, saya meminta penulis untuk memisahkan perubahan mekanis dari perubahan logis. Kirimkan pemindahan file dalam satu PR, perubahan logika sebenarnya dalam PR lainnya. Ini membuat kedua PR lebih mudah ditinjau dan lebih mudah untuk dibalik jika terjadi kesalahan.

Saya juga memperhatikan rasio kode yang ditambahkan dibandingkan dengan kode yang dihapus. Dalam pengalaman saya, PR terbaik memiliki jumlah baris bersih negatif—mereka mencapai sesuatu sambil membuat basis kode lebih kecil. Ketika saya melihat PR yang menambahkan 500 baris dan menghapus 50, saya mulai curiga. Apakah kita menambahkan kompleksitas? Apakah kita menduplikasi fungsionalitas yang sudah ada? Apakah penulis sadar apa yang sudah ada di basis kode?

Masalah ukuran juga berlaku untuk file individu. Jika sebuah PR menyentuh 30 file berbeda, bahkan jika total jumlah barisnya wajar, itu adalah bendera merah. Ini menunjukkan bahwa perubahannya either memiliki lingkup yang buruk atau basis kode memiliki masalah keterkaitan. Dalam hal apa pun, itu layak mendapat perhatian ekstra.

Lapisan Logika Bisnis: Tempat Kebanyakan Bug Bersembunyi

Setelah satu dekade tinjauan kode, saya bisa memberi tahu Anda di mana bug bersembunyi: di lapisan logika bisnis, khususnya dalam kasus-kasus tepi dan transisi status. Bukan di komponen UI. Bukan di kueri database. Di kode yang menerapkan aturan bisnis Anda yang sebenarnya.

Ketika saya meninjau logika bisnis, saya mencari asumsi. Setiap asumsi adalah potensi bug. Saya pernah meninjau kode untuk sistem kalkulasi diskon yang mengasumsikan semua diskon adalah persentase. Kode itu bekerja dengan sempurna sampai pemasaran ingin menawarkan promosi "$10 diskon". Sistemnya crash karena seseorang membagi dengan jumlah diskon, dan membagi dengan 10 ketika Anda mengharapkan angka antara 0 dan 1 akan menghasilkan hasil yang sangat salah.

Saya bertanya pada diri sendiri: apa yang terjadi di batas-batas? Apa yang terjadi jika inputnya nol? Apa yang terjadi jika negatif? Apa yang terjadi jika null? Apa yang terjadi jika string kosong versus string null? Apa yang terjadi jika array kosong? Apa yang terjadi jika pengguna tidak memiliki izin? Apa yang terjadi jika mereka memiliki semua izin? Apa yang terjadi jika database tidak mengembalikan hasil? Apa yang terjadi jika mengembalikan satu juta hasil?

Saya mencari angka ajaib dalam logika bisnis. Ketika saya melihat if (user.loginAttempts > 5), saya bertanya: mengapa 5? Apakah itu didokumentasikan? Apakah itu dapat dikonfigurasi? Apa yang terjadi jika kita ingin mengubahnya menjadi 3 untuk tipe pengguna tertentu? Angka ajaib dalam logika bisnis adalah utang teknis yang menunggu untuk terjadi.

Mesin keadaan adalah sumber umum bug lainnya. Ketika saya melihat kode yang mengelola transisi status—status pesanan, siklus hidup pengguna, tahap alur kerja—saya menggambar diagram keadaan dalam pikiran saya. Apakah semua transisi diperhitungkan? Dapatkah Anda masuk ke status yang tidak valid? Saya pernah menangkap bug di mana seorang pengguna bisa menjadi "aktif" dan "ditangguhkan" secara bersamaan karena kode yang menetapkan satu status tidak memeriksa yang lain.

Saya juga memperhatikan logika bisnis yang tersebar di beberapa lapisan. Jika saya melihat validasi di UI, lebih banyak validasi di API, dan masih lebih banyak validasi di lapisan database, saya tahu kita akan memiliki inkonsistensi. Aturan bisnis seharusnya tinggal di satu tempat, dan tempat itu seharusnya jelas.