Bagaimana membuat ulasan kode yang lebih baik saat permintaan tarik besar?

12

Penafian: Ada beberapa pertanyaan serupa, tetapi saya tidak menemukan sentuhan mana yang secara khusus masalah yang Anda hadapi saat meninjau permintaan tarik besar.

Masalah

Saya merasa ulasan kode saya dapat dilakukan dengan cara yang lebih baik. Saya terutama berbicara tentang ulasan kode besar dengan banyak perubahan di 20+ file.

Cukup mudah untuk menangkap masalah kode lokal yang jelas. Memahami apakah kode tersebut memenuhi kriteria bisnis adalah cerita yang berbeda.

Saya mengalami kesulitan mengikuti proses pemikiran pembuat kode. Ini cukup sulit ketika perubahannya banyak dan menyebar di beberapa file. Saya mencoba untuk fokus pada grup file yang terkait dengan bagian perubahan tertentu. Kemudian tinjau kelompok satu per satu. Sayangnya alat yang saya gunakan (Atlassian Bitbucket) tidak terlalu membantu. Setiap kali saya mengunjungi file, itu ditandai seperti yang terlihat, meskipun sering ternyata tidak terkait dengan bagian perubahan yang saat ini diperiksa. Belum lagi beberapa file harus dikunjungi beberapa kali dan perubahannya diulas satu per satu. Juga kembali ke file yang relevan ketika Anda mengikuti jalan yang buruk tidak mudah.

Kemungkinan solusi, dan mengapa mereka tidak berhasil untuk saya

Meninjau permintaan tarik dengan komit sering kali memecahkan masalah ukuran, tapi saya tidak suka karena saya akan sering melihat perubahan yang sudah usang.

Tentu saja, membuat permintaan tarikan yang lebih kecil sepertinya obat, tetapi itulah masalahnya, terkadang Anda mendapatkan permintaan tarikan yang besar dan harus ditinjau.

Anda juga dapat mengabaikan aspek logis dari kode secara keseluruhan, tetapi tampaknya cukup berisiko, terutama ketika kode tersebut berasal dari seorang programmer yang tidak berpengalaman.

Menggunakan alat yang lebih baik bisa membantu, tetapi saya tidak menemukannya.

Pertanyaan

  • Apakah Anda memiliki masalah yang serupa dengan ulasan kode Anda? Bagaimana Anda menghadapi mereka?
  • Mungkin Anda memiliki alat yang lebih baik?
Andrzej Gis
sumber
3
Mengapa ulasan kode begitu besar? Misalnya, jika itu adalah hasil dari refactoring otomatis, maka alih-alih meninjau komit, Anda memeriksa apakah mengulangi refactoring pada komit yang lebih lama menghasilkan salinan yang sama dari komit baru, dan kemudian memutuskan apakah Anda mempercayai alat tersebut atau tidak. Jadi, meninjau diff 1000-line tiba-tiba menjadi meninjau perintah 1-line dalam IDE dan keputusan apakah akan mempercayai vendor IDE.
Jörg W Mittag
Jika Anda memiliki keinginan untuk dapat melakukannya, buatlah tanggung jawab pembuat kode untuk mempermudah peninjauan kode. Ini berarti bahwa penulis harus berhati-hati dalam menekan komitmen yang tidak relevan, menulis ulang komitmen sehingga hanya berisi satu perubahan, untuk memisahkan komitmen refactoring utama, dan untuk memesan komitmen dengan cara yang masuk akal bagi pengulas.
Lie Ryan

Jawaban:

8

Kami memiliki masalah ini dan mengajukan pertanyaan di bawah ini telah bekerja dengan baik untuk kami:

Apakah PR melakukan satu hal yang dapat digabungkan dan dapat diuji secara independen?

Kami mencoba mendobrak hubungan masyarakat dengan tanggung jawab tunggal (SR). Setelah awal mendorong orang terkejut menemukan bahwa bahkan sesuatu yang kecil sekalipun tunggal dapat menjadi besar.

SR membuatnya sangat mudah untuk meninjau dan juga menyebarluaskan pengetahuan tentang implementasi yang diharapkan.

Ini juga memungkinkan untuk tambahan refaktor karena lebih banyak ditambahkan dan waktu penyelesaian PR berkurang secara drastis!

Saya sarankan memecah mereka oleh SR jika memungkinkan dan lihat apakah itu berhasil untuk Anda. Butuh latihan untuk melakukannya dengan cara itu.

PhD
sumber
11

Kadang-kadang Anda tidak dapat menghindari permintaan tarik besar - tetapi Anda dapat menentukan siapa yang memiliki tanggung jawab apa.

Saya memperlakukan permintaan tarik sebagai argumen persuasif. Penulis berusaha meyakinkan saya bahwa kode harus terlihat dan berfungsi seperti ini.

Seperti halnya argumen apa pun ia harus memiliki satu gagasan yang jelas. Ini baik:

  • sebuah refactor,
  • sebuah optimasi,
  • atau fungsionalitas baru.

Jika mereka tidak jelas, maka ada peluang bagus bahwa mereka tidak memahaminya sendiri. Buka dialog dan bantu mereka memecah argumen mereka menjadi sub-argumennya. Jika perlu, itu baik-baik saja - bahkan bermanfaat bagi mereka untuk menciptakan kembali komitmen tersebut, dan menawarkan permintaan tarik yang lebih komprehensif dan langsung.

Masih akan ada permintaan tarik besar, tetapi dengan argumen yang jelas akan lebih mudah untuk melihat apa yang tidak cocok.

Adapun alat, itu tergantung pada organisasi dan proses Anda. BitBucket adalah alat yang layak, apakah lebih baik atau tidak tergantung pada segala sesuatu mulai dari anggaran, perangkat keras, persyaratan, hingga proses yang sudah ada sebelumnya, aturan bisnis, dan berbagai penyesuaian perangkat lunak yang telah Anda buat untuk mengakomodasi BitBucket. Saya akan mulai dengan melihat-lihat dokumentasi untuk melihat apakah perilaku dapat dikonfigurasi, mungkin membuang ke komunitas plugin (atau bergabung dengan membuat plugin untuk melakukan itu).

Kain0_0
sumber
8

Jangan tinjau permintaan tarik lengkap, tetapi setiap komit. Anda akan mendapatkan pemahaman yang lebih baik tentang permintaan tarik dengan melakukannya dengan cara ini.¹

Jika komit kecil, melakukan tinjauan seperti itu seharusnya tidak menjadi masalah. Anda mengikuti perubahan satu per satu melalui komit, dan Anda akhirnya mendapatkan gambaran lengkap. Ada beberapa kelemahannya, seperti terkadang Anda akan meninjau perubahan yang akan dibatalkan beberapa saat kemudian, tetapi ini seharusnya tidak banyak.

Jika komit besar, maka Anda harus mendiskusikannya dengan orang yang melakukan komit itu. Jelaskan padanya mengapa komit besar bermasalah. Dengarkan argumen orang lain tentang mengapa ia jarang melakukan perubahan: Anda dapat belajar, misalnya, bahwa ia menghindari melakukan komitmen karena terlalu lama kaitannya dengan komitmen sebelumnya, dalam hal ini masalah ini harus diselesaikan terlebih dahulu.

Meninjau permintaan tarik dengan komit sering kali memecahkan masalah ukuran, tapi saya tidak suka karena saya akan sering melihat perubahan yang sudah usang.

Anda melakukannya, tetapi ini adalah masalah kecil, dan Anda harus membuang lebih sedikit waktu untuk meninjau kode yang akan dibatalkan beberapa kali lebih lambat daripada waktu yang Anda buang untuk mencari tahu mengapa kode itu diubah ketika meninjau satu file.

"Sering" tidak jelas, tetapi jika Anda mendapati diri Anda menghabiskan terlalu banyak waktu meninjau kode yang tidak menemukan jalannya ke revisi akhir permintaan tarik, Anda dapat menggunakan dua teknik:

  • Pergi cepat melalui semua komit sekali, hanya membaca pesan komit. Dengan cara ini, ketika mempelajari komit tertentu, Anda mungkin ingat bahwa pesan komit di suatu tempat kemudian mengatakan bahwa perubahan yang Anda lihat telah dikembalikan.

  • Memiliki tampilan berdampingan dari versi terbaru file (walaupun dalam banyak kasus, untuk komit besar, (1) file dapat sangat berbeda dan (2) file dapat diubah namanya atau blok kode yang besar dapat dipindahkan ke tempat lain, sehingga sulit menemukan file yang cocok).

  • Entah meminta komuter untuk menekan komit ketika itu masuk akal, atau memiliki konvensi pesan komit tertentu di mana satu komit membatalkan bagian dari komit lainnya, dan melakukan review Anda dengan mempertimbangkan beberapa komit berikut.


¹ Misalnya, bayangkan Anda sedang melihat file di mana beberapa variabel telah diubah namanya. Apa isinya? Bagaimana seharusnya Anda meninjau perubahan ini? Namun, jika Anda meninjaunya komit dengan komit, Anda akan melihat bahwa komit tunggal mengganti nama variabel yang sama dalam dua puluh file, dan komentar menunjukkan bahwa nama itu diubah untuk menggunakan istilah bisnis yang tepat. Perubahan ini sangat masuk akal dan memungkinkan untuk ditinjau.

Arseni Mourzenko
sumber
1
@ JörgWMittag: terima kasih atas komentar Anda. OP mengklaim bahwa dia tidak ingin melakukan tinjauan per-komit karena dia akan melihat perubahan yang sudah ketinggalan zaman, yang benar, tetapi seharusnya tidak menjadi masalah seperti memiliki semua masalah yang terkait dengan tinjauan per-file. Karena jawaban saya tidak jelas tentang itu, saya menambahkan bagian untuk secara khusus membahas hal ini.
Arseni Mourzenko
2

Cari tahu apa yang ingin Anda capai dengan ulasan permintaan tarik dan lihat apakah ada alternatif.

Misalnya Anda mungkin mau

  • Pastikan standar diikuti
  • Periksa fungsionalitas sudah benar
  • Pastikan lebih dari satu orang memahami kode tersebut
  • Latih junior

dll.

Beberapa di antaranya mungkin lebih baik dilayani oleh hal-hal lain, dan bahkan hanya dengan memahami alasannya Anda dapat membatasi cakupan cek Anda.

Misalnya, jika saya memeriksa setiap baris sehingga saya dapat menyarankan dan mendiskusikan perubahan untuk pelatihan, maka saya dapat mengabaikannya pada PR besar yang dilakukan oleh manula

Jika saya perlu memahami kodenya, mungkin melakukan pair programming pada fitur-fitur besar dan membatasi review kode pada pemeriksaan standar.

Anda tidak perlu memeriksa semua hal di setiap PR selama Anda memiliki pendekatan berlapis untuk QA

Ewan
sumber