Bagaimana menemukan hal-hal positif dalam ulasan kode?

184

Setelah beberapa masalah kualitas serius pada tahun lalu, perusahaan saya baru-baru ini memperkenalkan ulasan kode. Proses peninjauan kode diperkenalkan dengan cepat, tanpa pedoman atau jenis daftar periksa apa pun.

Pengembang lain dan saya di mana dipilih untuk meninjau semua perubahan yang dilakukan pada sistem, sebelum mereka digabungkan ke dalam trunk.

Kami juga terpilih sebagai "Pimpinan Teknis". Ini berarti kami bertanggung jawab atas kualitas kode, tetapi kami tidak memiliki wewenang untuk mengimplementasikan perubahan dalam proses, menetapkan kembali pengembang, atau menahan proyek.

Secara teknis kita bisa menolak merger, mengembalikannya ke pengembangan. Pada kenyataannya, ini hampir selalu berakhir dengan bos kami menuntut agar dikirimkan tepat waktu.

Manajer kami adalah MBA yang sebagian besar peduli dengan membuat jadwal proyek yang akan datang. Ketika dia mencoba, dia hampir tidak tahu apa yang dilakukan perangkat lunak kami dari sudut pandang bisnis, dan sedang berjuang untuk memahami bahkan permintaan pelanggan yang paling dasar tanpa penjelasan dari pengembang.

Saat ini pengembangan dilakukan di cabang-cabang pengembangan di SVN, setelah pengembang berpikir dia siap, dia menugaskan kembali tiket dalam sistem tiket kami ke manajer kami. Manajer kemudian memberikannya kepada kami.

Ulasan kode telah menyebabkan beberapa ketegangan dalam tim kami. Terutama beberapa anggota yang lebih tua mempertanyakan perubahan (Yaitu "Kami selalu melakukannya seperti ini" atau "Mengapa metode ini memiliki nama yang masuk akal, saya tahu apa artinya?").

Setelah beberapa minggu pertama, kolega saya mulai membiarkan segala sesuatunya merosot, tidak menyebabkan masalah dengan rekan kerja (dia mengatakan kepada saya sendiri, bahwa setelah laporan bug diajukan oleh seorang pelanggan, bahwa dia tahu bug itu, tetapi takut bahwa pengembang akan marah padanya karena menunjukkannya).

Saya, di sisi lain, sekarang dikenal sebagai keledai karena menunjukkan masalah dengan kode yang dilakukan.

Saya tidak berpikir standar saya terlalu tinggi.

Daftar periksa saya saat ini adalah:

  • Kode akan dikompilasi.
  • Setidaknya ada satu cara kode akan bekerja.
  • Kode ini akan berfungsi pada sebagian besar kasus normal.
  • Kode ini akan bekerja dengan sebagian besar kasus tepi.
  • Kode akan memunculkan pengecualian yang masuk akal jika data yang dimasukkan tidak valid.

Tetapi saya sepenuhnya menerima tanggung jawab dari cara saya memberikan umpan balik. Saya sudah memberikan poin yang bisa ditindaklanjuti menjelaskan mengapa sesuatu harus diubah, kadang-kadang bahkan hanya bertanya mengapa sesuatu diterapkan dengan cara tertentu. Ketika saya pikir itu buruk, saya menunjukkan bahwa saya akan mengembangkannya dengan cara lain.

Yang kurang dari saya adalah kemampuan untuk menemukan sesuatu yang menunjukkan "baik". Saya membaca bahwa orang harus mencoba untuk menyampaikan berita buruk dalam berita baik.

Tetapi saya mengalami kesulitan untuk menemukan sesuatu yang baik. "Hei, kali ini kamu benar-benar melakukan semua yang kamu lakukan" lebih merendahkan daripada menyenangkan atau membantu

Ulasan Kode Contoh

Hai Joe,

Saya memiliki beberapa pertanyaan tentang perubahan Anda di Library \ ACME \ ExtractOrderMail Class.

Saya tidak mengerti mengapa Anda menandai "TempFilesToDelete" sebagai statis? Saat ini panggilan kedua ke "GetMails" akan mengeluarkan pengecualian, karena Anda menambahkan File tetapi tidak pernah menghapusnya, setelah Anda menghapusnya. Saya tahu bahwa fungsi ini hanya dipanggil sekali per jalankan, tetapi di masa depan ini mungkin berubah. Bisakah Anda membuatnya menjadi variabel instan, lalu kita bisa memiliki beberapa objek secara paralel.

... (Beberapa poin lain yang tidak berfungsi)

Poin kecil:

  • Mengapa "GetErrorMailBody" menggunakan Pengecualian sebagai Parameter? Apakah saya melewatkan sesuatu? Anda tidak melempar pengecualian, Anda hanya meneruskannya dan memanggil "ToString". Mengapa demikian?
  • SaveAndSend Bukan nama yang bagus untuk Metode ini. Metode ini mengirim pesan kesalahan jika pemrosesan email salah. Bisakah Anda mengganti nama menjadi "SendErrorMail" atau yang serupa?
  • Tolong jangan hanya komentar kode lama, hapus saja. Kami masih memilikinya di subversi.
RobMMurdock
sumber
8
Tolong jangan sajikan sandwich $ h! T untuk mencapai keseimbangan mitos baik dan buruk. Jika mereka telah melakukan sesuatu yang baik, beri tahu mereka, jika mereka telah melakukan sesuatu yang perlu diperbaiki, beri tahu mereka. Mencampur baik dan buruk mencairkan pesan. Jika mereka mendapat lebih banyak umpan balik negatif daripada positif, mungkin mereka akan menyadari bahwa mereka perlu berubah. Pendekatan sandwich Anda memberikan rasio 2: 1 untuk setiap negatif, sehingga akhirnya positif, adalah pesan yang ingin Anda kirim.
cdkMoose
14
Hentikan penggunaan orang kedua. Kode adalah subjek, bukan kode. Misalnya, tulis: SaveAndSend harus diganti namanya agar lebih sesuai dengan perilakunya, seperti misalnya SendErrorMail . Saat ini, itu benar-benar terlihat seperti Anda memberi perintah kepada kolega Anda, bahkan dengan semua "tolong" Anda tumpah ke mana-mana. Saya tidak akan menerimanya dari pengulas. Saya jauh lebih suka seseorang yang langsung menyatakan "Ini harus dilakukan", daripada meminta saya (bahkan dengan sopan) untuk melakukan sesuatu.
Arthur Havlicek
4
"Saya membaca bahwa seseorang harus mencoba untuk mengapit berita buruk dengan berita baik." Anda perlu memastikan bahwa ada pemahaman global yang jelas bahwa ini bukanlah tinjauan kode. Itu tidak seperti ulasan kinerja karyawan atau ulasan film, yang menimbang baik dan buruk. Mereka lebih seperti bagian dari proses QA. Anda tidak akan mengharapkan penguji Anda untuk membuat tiket yang mengatakan "Fitur ini hebat, dan berfungsi seperti yang saya harapkan!", Dan Anda seharusnya tidak mengharapkannya dalam ulasan kode.
Ben Aaronson
3
Saya pikir langkah pertama Anda harus dalam menciptakan seperangkat standar standar / pedoman kode, memungkinkan anggota lain untuk memberikan umpan balik, dan pada dasarnya mendapatkan "persetujuan" dari semua orang bahwa pedoman tersebut "sesuai alasan". Kemudian, mereka semua sadar bahwa mereka setuju untuk ditahan. Itu bekerja dengan baik di prev. perusahaan tempat saya bekerja.
code_dredd
3
Jangan gunakan kalimat ini "tetapi di masa depan ini mungkin berubah." Kode hanya untuk apa yang dibutuhkan sekarang. Jangan membuat kompleksitas untuk perubahan di masa depan yang mungkin atau mungkin tidak terjadi. Jika Anda benar-benar tahu itu akan berubah itu berbeda, tetapi tidak untuk kesempatan bahwa itu akan berubah.
House of Dexter

Jawaban:

124

Bagaimana menemukan hal-hal positif dalam ulasan kode?

Setelah beberapa masalah kualitas serius pada tahun lalu, perusahaan saya baru-baru ini memperkenalkan ulasan kode.

Hebat, Anda memiliki peluang nyata untuk menciptakan nilai bagi perusahaan Anda.

Setelah beberapa minggu pertama, kolega saya mulai membiarkan segala sesuatunya merosot, untuk tidak menyebabkan masalah dengan rekan kerja (dia mengatakan kepada saya sendiri, bahwa setelah laporan bug diajukan oleh seorang pelanggan, bahwa dia tahu bug itu, tetapi takut bahwa pengembang akan marah padanya karena menunjukkannya).

Rekan kerja Anda seharusnya tidak melakukan peninjauan kode jika ia tidak dapat memberi tahu pengembang apa yang salah dengan kode mereka. Adalah tugas Anda untuk menemukan masalah dan menyelesaikannya sebelum memengaruhi pelanggan.

Demikian juga, seorang pengembang yang mengintimidasi rekan kerja meminta dipecat. Saya merasa terintimidasi setelah peninjauan kode - saya memberi tahu bos saya, dan itu ditangani. Juga, saya menyukai pekerjaan saya, jadi saya terus menerima umpan balik, positif dan negatif. Sebagai pengulas, itu saya, bukan orang lain.

Saya, di sisi lain, sekarang dikenal sebagai keledai karena menunjukkan masalah dengan kode yang dilakukan.

Sayangnya, Anda mengatakan Anda bersikap bijaksana. Anda dapat menemukan lebih banyak untuk dipuji, jika Anda memiliki lebih banyak untuk dicari.

Mengkritik kode, bukan pembuatnya

Anda memberi contoh:

Saya memiliki beberapa pertanyaan tentang perubahan Anda di

Hindari menggunakan kata-kata "kamu" dan "milikmu", katakan, "perubahan" sebagai gantinya.

Apakah saya melewatkan sesuatu? [...] Mengapa demikian?

Jangan menambahkan retorika berkembang ke kritik Anda. Jangan bercanda juga. Ada aturan yang saya dengar, "Jika Anda merasa senang mengatakan, jangan katakan itu, itu tidak baik."

Mungkin Anda menggosok ego Anda sendiri atas biaya orang lain. Simpan hanya dengan fakta.

Angkat bilah dengan memberikan umpan balik positif

Ini meningkatkan standar untuk memuji sesama pengembang Anda ketika mereka memenuhi standar yang lebih tinggi. Jadi itu berarti pertanyaan,

Bagaimana menemukan hal-hal positif dalam ulasan kode?

itu bagus, dan layak ditangani.

Anda dapat menunjukkan di mana kode memenuhi cita-cita praktik pengkodean tingkat yang lebih tinggi.

Carilah mereka untuk mengikuti praktik terbaik, dan terus meningkatkan standar. Setelah cita-cita yang lebih mudah menjadi harapan semua orang, Anda akan ingin berhenti memuji ini dan mencari praktik pengkodean yang lebih baik untuk pujian.

Praktik terbaik khusus bahasa

Jika bahasa mendukung dokumentasi dalam kode, ruang nama, berorientasi objek atau fitur pemrograman fungsional, Anda dapat memanggil mereka dan memberi selamat kepada penulis untuk menggunakannya jika diperlukan. Hal-hal ini biasanya termasuk dalam pedoman gaya:

  • Apakah memenuhi standar panduan gaya bahasa in-house?
  • Apakah itu memenuhi panduan gaya yang paling otoritatif untuk bahasa (yang mungkin lebih ketat daripada in-house - dan dengan demikian masih sesuai dengan gaya in-house)?

Praktik terbaik umum

Anda dapat menemukan poin untuk memuji prinsip-prinsip pengkodean generik, di bawah berbagai paradigma. Misalnya, apakah mereka memiliki persatuan yang baik? Apakah unittests menutupi sebagian besar kode?

Mencari:

  • unit test yang hanya menguji fungsionalitas subjek - mengejek fungsionalitas mahal yang tidak dimaksudkan untuk diuji.
  • tingkat cakupan kode yang tinggi, dengan pengujian lengkap terhadap API dan fungsionalitas publik semantik.
  • tes penerimaan dan tes asap yang menguji fungsionalitas ujung ke ujung, termasuk fungsionalitas yang dipermainkan untuk tes unit.
  • penamaan yang baik, data poin kanonik sehingga kode KERING (Jangan Ulangi Diri Sendiri), tidak ada string atau angka ajaib.
  • penamaan variabel dilakukan dengan sangat baik sehingga komentar sebagian besar berlebihan.
  • pembersihan, peningkatan obyektif (tanpa pengorbanan), dan refactoring yang sesuai yang mengurangi garis kode dan utang teknis tanpa membuat kode benar-benar asing bagi penulis asli.

Pemrograman Fungsional

Jika bahasanya fungsional, atau mendukung paradigma fungsional, cari cita-cita ini:

  • menghindari global dan negara global
  • menggunakan fungsi penutupan dan sebagian
  • fungsi kecil dengan nama yang dapat dibaca, benar, dan deskriptif
  • titik keluar tunggal, meminimalkan jumlah argumen

Pemrograman Berorientasi Objek (OOP)

Jika bahasa ini mendukung OOP, Anda dapat memuji penggunaan yang sesuai dari fitur-fitur ini:

  • enkapsulasi - menyediakan antarmuka publik yang didefinisikan dengan rapi dan kecil, dan menyembunyikan detailnya.
  • inheritance - code digunakan kembali dengan tepat, mungkin melalui mixin.
  • polimorfisme - antarmuka didefinisikan, mungkin kelas dasar abstrak, fungsi yang ditulis untuk mendukung polimorfisme parametrik.

di bawah OOP, ada juga prinsip SOLID (mungkin beberapa redundansi ke fitur OOP):

  • tanggung jawab tunggal - setiap objek memiliki satu pemangku kepentingan / pemilik
  • buka / tutup - tidak mengubah antarmuka objek yang ditetapkan
  • Substitusi Liskov - subclass dapat diganti untuk contoh orang tua
  • segregation antarmuka - antarmuka yang disediakan oleh komposisi, mungkin mixin
  • inversi ketergantungan - antarmuka yang ditentukan - polimorfisme ...

Prinsip pemrograman Unix :

Prinsip-prinsip Unix adalah modularitas, kejelasan, komposisi, pemisahan, kesederhanaan, kekikiran, transparansi, kekokohan, representasi, kejutan, keheningan, perbaikan, ekonomi, pembangkitan, optimalisasi, keragaman, dan ekstensibilitas.

Secara umum, prinsip-prinsip ini dapat diterapkan di bawah banyak paradigma.

Kriteria Anda

Ini terlalu sepele - saya akan merasa direndahkan jika dipuji karena ini:

  • Kode akan dikompilasi.
  • Setidaknya ada satu cara kode akan bekerja.
  • Kode ini akan berfungsi pada sebagian besar kasus normal.

Di sisi lain, ini adalah pujian yang cukup tinggi, mengingat apa yang tampaknya Anda hadapi, dan saya tidak akan ragu untuk memuji pengembang karena melakukan ini:

  • Kode ini akan bekerja dengan sebagian besar kasus tepi.
  • Kode akan memunculkan pengecualian yang masuk akal jika data yang dimasukkan tidak valid.

Menuliskan aturan untuk meneruskan tinjauan kode?

Namun, itu ide yang bagus dalam teori, sementara saya biasanya tidak akan menolak kode untuk penamaan yang buruk, saya telah melihat penamaan yang sangat buruk sehingga saya akan menolak kode dengan instruksi untuk memperbaikinya. Anda harus dapat menolak kode untuk alasan apa pun.

Satu-satunya aturan yang dapat saya pikirkan untuk menolak kode adalah tidak ada yang begitu mengerikan sehingga saya akan membuatnya keluar dari produksi. Nama yang sangat buruk adalah sesuatu yang saya ingin hindari produksi - tetapi Anda tidak dapat membuat aturan itu.

Kesimpulan

Anda dapat memuji praktik-praktik terbaik yang diikuti di bawah berbagai paradigma, dan mungkin di bawah semuanya, jika bahasa mendukungnya.

Aaron Hall
sumber
8
Saya bahkan berpendapat bahwa banyak dari ini bisa menjadi judul pada template umpan balik ulasan kode. Ini memungkinkan peluang untuk komentar seperti "pekerjaan hebat" di bawah banyak judul, tanpa biaya tambahan nyata. Ini juga memberi kolega ide bagus tentang bagaimana membuat kode mereka lebih baik.
Stephen
9
Saat mendaftar banyak praktik yang baik, Anda mungkin menjawab pertanyaan yang salah - karena itu benar-benar masalah. Dan sulit untuk menemukan sistem peninjauan yang memungkinkan umpan balik tersebut. Hal-hal penting disembunyikan dalam kebisingan yang tidak berguna. Terkadang, jawaban atas sebuah pertanyaan hanyalah "Jangan lakukan itu - itu adalah cara yang salah. Masalah Anda ada di tempat lain dan harus diselesaikan dengan baik." Jika orang mulai fokus untuk menemukan hal-hal yang baik, tinjauan kode menjadi buang-buang waktu. Anda dapat memberi tahu rekan kerja Anda saat makan siang betapa menyenangkan implementasinya, dan dia mungkin menghargainya.
Eiko
4
@ Harun: Setuju dengan Anda dalam pendekatan. Banyak jawaban di sini mengatakan "jangan gula itu", tapi saya mengerti ini bukan tentang menyenangkan semua orang. Orang lebih cenderung mengikuti pendekatan yang baik ketika hal-hal baik yang mereka lakukan diperkuat, bukan ketika mereka diberitahu bahwa mereka salah. Kunci mereka di sini adalah bersikap bijaksana tetapi konsisten tentang apa yang harus dilakukan. Dari deskripsi OP, ia berada dalam tim pengkodean yang kurang sempurna, dengan anggota lama yang terbiasa dengan cara mereka. Mereka akan lebih mudah menerima pendekatan yang lembut.
Hoàng Long
@ HoàngLong Tidak setiap 'timer lama' akan selalu menjadi 'lebih reseptif'. Selalu ada seseorang yang tidak masuk akal di suatu tempat. Sebagai contoh, saya dulu bekerja dengan seorang pria yang bersikeras 'porting' praktik terbaiknya Perl ke Python dan Subversion ke Git, dan memiliki beberapa jenis keluhan setiap kali dipanggil, terlepas dari bagaimana itu, bahkan jika alasan dijelaskan. Karena pada saat itu tanggung jawab untuk yang jatuh di pangkuan saya (saya adalah satu-satunya yang berpengalaman dengan Python dan Git), saya pikir beberapa orang mungkin merasa terancam (?) Dan bereaksi sesuai ...
code_dredd
104

Jangan repot-repot memilih sesuatu yang baik kecuali itu adalah contoh padat dan langsung terkait dengan masalah yang difokuskan.

Saya tidak akan menutup-nutupi hal itu - dari suaranya Anda berurusan dengan setidaknya satu orang yang merasa tidak aman dengan kemampuan mereka dan sedang menangani ditantang tentang pekerjaan mereka dengan cara yang tidak dewasa. Mereka juga cenderung buruk dalam pekerjaan mereka - pengembang yang baik harus selalu bersedia untuk mencerminkan diri sendiri, menerima kritik yang membangun, dan terbuka untuk mengubah cara mereka.

Sekarang yang ada di udara mari kita bicara tentang Anda. Terlepas dari apakah Anda berpikir Anda masuk akal Anda harus ekstra hati-hati dengan orang-orang seperti ini untuk mendapatkan bola bergulir. Saya telah menemukan cara terbaik untuk berurusan dengan orang-orang ini adalah sangat berhati-hati dengan cara Anda mengatakan sesuatu.

Pastikan Anda menyalahkan kode dan bukan pembuatnya . Fokus pada satu masalah yang dihadapi daripada kotoran yang merupakan basis kode Anda, yang mungkin memiliki andil besar dalam pembuatan dan akan dilihat sebagai serangan pribadi lebih lanjut. Pilih pertempuran Anda pada awalnya, fokus pada masalah kritis yang memanifestasikan diri kepada pengguna Anda sehingga Anda tidak meluncurkan serangkaian kritik pada orang yang memimpin mereka untuk mengabaikan semua yang Anda katakan.

Bahasa tubuh dan nada penting jika Anda berbicara langsung dengan mereka, jelas dengan apa yang Anda katakan dan pastikan Anda tidak berbicara dengan mereka atau mengabaikan kemampuan teknis mereka. Mereka kemungkinan besar akan berada di posisi bertahan langsung sehingga Anda perlu menyelesaikan kekhawatiran mereka alih-alih mengonfirmasikannya. Anda harus sadar tentang percakapan ini tanpa terlalu jelas sehingga mereka secara tidak sadar berpikir Anda ada di pihak mereka, dan mudah-mudahan mereka menerima bahwa mereka perlu membuat perubahan yang dibawa ke perhatian.

Jika ini tidak berhasil, Anda bisa mulai sedikit lebih agresif. Jika produk dapat dijalankan dari ruang konferensi, angkat di proyektor selama peninjauan kode dan tunjukkan bug secara langsung, lebih baik jika manajer ada di sana sehingga orang tersebut tidak dapat mundur. Ini bukan untuk mempermalukan mereka tetapi untuk memaksa mereka menerima bahwa masalahnya adalah nyata bagi pengguna dan bahwa itu perlu diperbaiki, bukan hanya keluhan yang Anda miliki dengan kode mereka.

Akhirnya jika Anda tidak mendapatkan apa-apa, bosan memperlakukan orang seperti siswa taman kanak-kanak, dan manajemen sama sekali tidak menyadari masalah tersebut, dan itu juga mencerminkan buruknya kinerja Anda sebagai peninjau kode atau Anda peduli dengan kesejahteraan Anda. perusahaan dan / atau produk, Anda perlu mulai berbicara dengan bos Anda tentang perilaku mereka. Jadilah sespesifik dan sespesonif mungkin - buat kasus bisnis bahwa produktivitas dan kualitas menderita.

plast1k
sumber
4
Strategi lain yang saya temukan bermanfaat sebagai peninjau dan sebagai seseorang yang ditinjau adalah untuk mengatributkan kebutuhan untuk menutupi kasus tepi, karena pihak ketiga. Saya minta maaf kepada mereka yang berada di posisi manajemen, tetapi mengatakan hal-hal seperti "kita perlu menjelaskan kasus tepi ini karena manajemen benar-benar mengendarai kita, jadi kami ingin memastikan ini dekat dengan bukti peluru. Memberi mereka rasa nyaman." Juga terdengar seperti manajemen tidak keberatan menjadi "orang jahat" dalam kasus OP.
Greg Burghardt
7
@GregBurghardt Hei, mereka tidak menyebutnya politik kantor tanpa alasan.
plast1k
30
Saya setuju dengan apa yang Anda katakan di sini, dan ini semakin jauh, tetapi saya pikir penting untuk diingat bahwa ulasan kode tidak seharusnya bermusuhan. Mereka adalah dua orang yang duduk dengan tujuan bersama untuk membuat kode yang baik dan produk yang baik. Tidak apa-apa bagi Anda untuk tidak setuju kadang-kadang tentang apakah satu pendekatan atau yang lain lebih baik, tetapi semua argumen di kedua sisi harus berakar dalam melakukan hal yang benar untuk tim, perusahaan, dan / atau pelanggan. Jika Anda berdua bisa setuju untuk itu, ini adalah proses yang lebih lancar.
hobbs
6
"Jangan repot-repot memilih sesuatu yang baik kecuali ini contoh padat yang solid dan berhubungan langsung dengan masalah fokus." - Saya pikir pembukaan itu agak keras. Ketika saya melakukan review kode saya selalu "repot" untuk memulai dengan sesuatu yang positif, bahkan saya harus menggunakan sesuatu yang jinak. Ini membantu mengatur nada, dan menunjukkan Anda tidak hanya mencari aspek negatif dari kode.
Bryan Oakley
2
"Pastikan Anda menyalahkan kode dan bukan pembuatnya". Setuju, tetapi jenis tidak aman / belum dewasa tidak akan mengambil jalan itu.
MetalMikester
95

Ulasan kode bisa menjadi racun, buang-buang waktu, keinginan untuk hidup nerd perang sadap. Lihat saja perbedaan pendapat tentang hal-hal seperti kode bersih vs komentar, konvensi penamaan, pengujian unit dan integrasi, strategi check in, RESTfulness, dll., Dll.

Satu-satunya cara untuk memastikan Anda menghindari ini adalah dengan menuliskan aturan untuk lulus tinjauan kode.

Maka itu bukan orang yang gagal atau menyetujui checkin. Mereka hanya memeriksa bahwa aturan telah dipatuhi.

Dan karena mereka ditulis di muka, ketika Anda menulis kode Anda, Anda dapat mengikuti aturan dan tidak harus menjelaskan alasan Anda atau berargumentasi nanti.

Jika Anda tidak menyukai aturan tersebut, adakan pertemuan untuk mengubahnya.

Ewan
sumber
56
"Satu-satunya cara untuk memastikan Anda menghindari ini adalah dengan menuliskan aturan untuk lulus ulasan kode." Ini. Anda harus meninjau semuanya dengan beberapa standar yang telah ditetapkan untuk proyek secara keseluruhan, tidak bertentangan dengan ide-ide pribadi Anda tentang apa yang baik-baik saja, namun ide-ide pribadi Anda mungkin berwawasan luas.
alephzero
6
Pertanyaannya adalah bagaimana menemukan hal-hal positif. Bagaimana Anda tahu nama itu cukup baik? Kapan nama terlalu buruk untuk lulus ulasan kode? Banyak hal yang bisa dipujinya terlalu subyektif untuk dapat memiliki aturan yang keras dan cepat. Karena itu, saya tidak berpikir ini menjawab pertanyaan.
Aaron Hall
20
-1 Saya suka cara Anda melompat dari mengkritik "perang nerd" dan kemudian mengatakan "Satu-satunya cara untuk memastikan Anda menghindari ini".
tymtam
33
Tidak mungkin untuk menuliskan aturan untuk setiap keputusan desain yang buruk. Dan jika Anda mencoba membuat satu saat Anda pergi, Anda akan segera menemukan bahwa dokumen menjadi tidak dapat digunakan karena panjangnya. -1
jpmc26
15
Jauh lebih bermanfaat daripada standar pengkodean adalah pengembang dan pengulas yang dapat bertindak sebagai orang dewasa yang tepat.
gnasher729
25

Saya tidak akan melapisi tanggapan Anda karena hal itu dapat dianggap menggurui.

Menurut pendapat saya, praktik terbaik adalah selalu fokus pada kode dan tidak pernah pada penulis.

Ini adalah review kode , bukan review pengembang , jadi:

  • "Loop sementara ini mungkin tidak pernah selesai", bukan "Loop Anda mungkin tidak pernah selesai"
  • "Sebuah test case diperlukan untuk skenario X", bukan "Anda tidak menulis tes untuk menutup skenario ini"

Juga sangat penting untuk menerapkan aturan yang sama saat berbicara tentang ulasan dengan yang lain:

  • "Anne, apa pendapatmu tentang kode ini?", Bukan "Anne, bagaimana menurutmu tentang kode Jon?"

Peninjauan kode bukan waktu untuk peninjauan kinerja - itu harus dilakukan secara terpisah.

tymtam
sumber
3
Anda sebenarnya tidak menjawab pertanyaan itu. Pertanyaannya adalah "Bagaimana menemukan hal-hal positif dalam ulasan kode?" - dan jawaban ini hanya kontradiksi - Anda menjawab, "bagaimana saya memberikan umpan balik negatif".
Aaron Hall
15

Saya terkejut belum disebutkan dalam jawaban apa pun sejauh ini, dan mungkin pengalaman saya dengan ulasan kode adalah yang tidak biasa, tetapi:

Mengapa Anda meninjau seluruh permintaan penggabungan dalam satu pesan?

Pengalaman saya dengan ulasan kode adalah melalui GitLab; Saya selalu membayangkan bahwa alat peninjau kode lainnya akan bekerja dengan cara yang sama.

Ketika saya memberikan ulasan kode, saya mengomentari baris-baris spesifik dan individual dari diff. Ini sangat tidak mungkin untuk diterima sebagai kritik pribadi, karena ini adalah komentar pada kode - dan sebenarnya ditampilkan sebagai komentar pada kode, ditampilkan langsung di bawah kode itu adalah komentar pada.

Saya juga bisa mengomentari seluruh permintaan penggabungan, dan sering kali melakukannya. Tetapi poin-poin spesifik dapat ditunjukkan pada garis-garis spesifik dari diff. (Juga, ketika komit baru ditambahkan sedemikian rupa sehingga diff berubah, komentar pada "diff kedaluwarsa" disembunyikan secara default.)

Dengan alur kerja ini, ulasan kode menjadi jauh lebih modular dan lebih tidak kohesif. Sebuah baris dari tinjauan kode hanya dapat mengatakan,

Pendekatan yang bagus, membungkus ini dalam fungsi khusus!

Atau bisa dikatakan,

Nama objek ini tidak benar-benar cocok dengan tujuan objek; mungkin kita bisa menggunakan nama seperti 'XYZ' saja?

Atau jika ada masalah besar dengan bagian, saya dapat menulis:

Saya melihat bahwa kode ini berfungsi (dan saya mengerti mengapa ia bekerja) tetapi membutuhkan studi terfokus untuk memahaminya. Bisakah Anda refactor menjadi fungsi yang terpisah sehingga akan lebih mudah untuk mempertahankannya di masa depan?

(Contoh: Fungsi ABC sebenarnya melakukan tiga hal di sini: bazzing the foo, melarang boz dan fripping the zorf. Itu semua bisa menjadi fungsi yang terpisah.)

Setelah menulis semua hal di atas, saya dapat membuat ringkasan komentar tentang seluruh permintaan penggabungan, seperti:

Ini adalah fitur baru yang hebat dan akan sangat berguna setelah digabungkan. Bisakah Anda membersihkan penamaan fungsi, dan menangani refactoring yang disebutkan dalam komentar individu yang saya buat, dan kemudian beri tahu saya untuk melihatnya lagi? :)


Bahkan jika permintaan penggabungan adalah sarapan anjing lengkap, komentar masing-masing dapat sederhana. Akan ada lebih banyak dari mereka. Maka komentar ringkasan mungkin mengatakan:

Maaf, tapi kode ini tidak terlalu buruk. Ada banyak sekali kasus tepi (seperti yang dijelaskan dalam komentar individu) yang akan ditangani secara tidak benar dan memberikan pengalaman pengguna yang buruk, atau bahkan korupsi data dalam satu kasus. (Lihat komentar pada commit 438a95fb734.) Bahkan beberapa kasus penggunaan normal akan menghasilkan kinerja aplikasi yang sangat buruk (spesifik yang dicatat dalam komentar individu pada diff untuk somefile.c).

Agar siap berproduksi, fitur ini akan membutuhkan penulisan ulang penuh dengan lebih banyak perhatian diberikan pada asumsi apa yang aman dibuat pada setiap titik dalam aliran. (Petunjuk: Tidak ada asumsi yang aman kecuali jika sudah diperiksa.)

Saya menutup permintaan gabungan sambil menunggu penulisan ulang penuh.


Ringkasan: Tinjau aspek teknis dari kode sebagai komentar pada setiap baris kode. Kemudian rangkum komentar-komentar itu dalam komentar keseluruhan pada permintaan penggabungan. Jangan menjadi pribadi — hanya berurusan dengan fakta, dan menurut pendapat Anda tentang kode , bukan tentang pembuat kode. Dan mendasarkan pendapat Anda pada fakta, pengamatan yang akurat, dan pemahaman.

Wildcard
sumber
12

Saya benar-benar terkejut tidak ada yang mengambilnya, tetapi ada yang salah dengan ulasan sampel yang diposting.

Tidak ada alasan Anda harus menghubungi Joe secara langsung. Joe memperbaiki kesalahannya bukan urusanmu. Itu seseorang tidak, adalah bisnis Anda. Kekhawatiran Anda adalah kualitas kode. Jadi, alih-alih menulis permintaan / pesanan / permintaan (bahwa, jika saya Joe, saya bisa menolak karena Anda tidak sah untuk ini), tetaplah di peran Anda dan tulis daftar todo anonim sederhana.

Berusaha bersikap adil dalam memberikan poin yang baik dan buruk bukan hanya tidak mungkin tetapi sepenuhnya di luar peran Anda.

Berikut adalah contoh reformulasi dengan konten yang diambil dari ulasan Anda:

  • Sebelum kita menarik perubahan di Kelas Library \ ACME \ ExtractOrderMail kita perlu memperbaiki beberapa masalah.
  • Kecuali saya melewatkan sesuatu, "TempFilesToDelete" seharusnya tidak statis.
  • Di masa depan, kita dapat memanggil fungsi lebih dari sekali per jalankan, ini sebabnya kita perlu (apa yang harus dilakukan di sini).
  • Saya perlu memahami mengapa "GetErrorMailBody" mengambil Pengecualian sebagai Parameter. (Dan, saya menjadi batas di sini, karena sekarang, Anda seharusnya sudah memiliki kesimpulan )
  • SaveAndSend harus diganti namanya agar lebih sesuai dengan perilakunya, seperti misalnya ke "SendErrorMail"
  • Kode yang dikomentari harus dihapus untuk tujuan keterbacaan. Kami menggunakan subversi untuk rollback akhirnya.

Jika Anda merumuskan ulasan seperti ini, tidak peduli seberapa besar pembaca membenci Anda secara pribadi, yang dapat ia lihat di sini hanyalah catatan tentang perbaikan yang harus dilakukan seseorang di kemudian hari. Siapa ? Kapan ? Tidak ada yang peduli. Apa ? Mengapa ITULAH yang harus Anda katakan.

Sekarang, Anda akan membahas alasan ulasan kode meningkatnya ketegangan dengan mengambil faktor manusia dari persamaan.

Arthur Havlicek
sumber
Ulasan sampel adalah tambahan terbaru untuk pertanyaan, sebagian besar penjawab belum melihatnya
Izkata
8

Inti dari tinjauan kode adalah untuk menemukan masalah. Jika ada setiap bug, hal terbaik yang dapat Anda lakukan adalah menulis kasus pengujian yang gagal.

Tim Anda (manajer) harus berkomunikasi bahwa membuat bug adalah bagian dari permainan, tetapi menemukan dan memperbaikinya akan menyelamatkan pekerjaan semua orang .

Mungkin bermanfaat untuk melakukan pertemuan rutin (baik tim atau pasangan) dan membahas beberapa masalah. Pemrogram asli tidak sengaja memperkenalkan masalah, dan kadang-kadang dia mungkin berpikir itu cukup baik (dan kadang-kadang bahkan mungkin). Tetapi memiliki sepasang mata yang kedua memberikan pandangan yang benar-benar segar, dan dia mungkin belajar banyak dari melihat masalahnya.

Ini benar-benar masalah budaya, dan butuh banyak kepercayaan dan komunikasi. Dan waktu untuk bekerja dengan hasilnya.

Eiko
sumber
2
"Inti dari tinjauan kode adalah untuk menemukan masalah" benar - tetapi tidak satu pun dari ini menjawab pertanyaan yang diajukan.
Aaron Hall
3
Dia menanyakan masalah- x
Eiko
1
Tim Anda (manajer) harus berkomunikasi bahwa membuat bug adalah bagian dari permainan, tetapi menemukan dan memperbaikinya akan menyelamatkan pekerjaan semua orang . Ini benar dan itu berarti bahwa setiap orang adalah pemangku kepentingan. Tetapi seharusnya tidak menjadi tanggung jawab seseorang yang menunjukkan bug (atau, kode spaghetti yang buruk) untuk menulis test case untuk membuktikan kepada pembuat kode asli bahwa itu adalah bug. (hanya jika itu secara luas diperdebatkan bahwa itu benar-benar adalah bug.)
robert bristow-johnson
6

Saya pikir hal positif yang harus dilakukan adalah mendapatkan konsensus tentang standar pengkodean sebelum melakukan tinjauan. Orang lain cenderung membeli lebih banyak untuk sesuatu ketika mereka memiliki input.

Untuk sesuatu seperti konvensi penamaan, tanyakan kepada orang lain apakah ini penting. Sebagian besar pengembang akan setuju terutama di antara rekan-rekan mereka. Siapa yang ingin menjadi orang yang tidak ingin setuju dengan sesuatu yang begitu lazim di dunia pemrograman? Ketika Anda memulai proses dengan memilih kode seseorang dan menunjukkan kelemahannya, mereka menjadi sangat defensif. Ketika standar ditetapkan, akan ada ketidaksepakatan pada interpretasi atau apa yang dianggap "cukup baik," tetapi Anda lebih baik daripada Anda sekarang.

Bagian lain dari proses ini adalah menentukan bagaimana ulasan kode akan menangani keberatan. Ini tidak bisa menjadi perdebatan tanpa akhir. Pada titik tertentu, seseorang harus membuat keputusan. Mungkin ada pihak ketiga untuk menjadi hakim atau peninjau mendapatkan semua kekuatan. Hal-hal yang harus dilakukan harus menjadi tujuan.

Bagian terakhir dari ini adalah untuk membuat semua orang tahu bahwa Ulasan Kode bukan ide Anda, mereka akan tetap, jadi semua orang harus berusaha untuk membuat yang terbaik dari itu. Jika semua orang merasa tidak berdaya, mungkin mereka dapat diizinkan meninjau kode Anda?

Mudah-mudahan, beberapa hasil yang terukur bagi manajemen adalah membatasi bug, keluhan pelanggan, keterlambatan dll. Jika tidak, seseorang hanya mendengar kata kunci "tinjauan kode" dan berpikir jika mereka hanya menambahkannya ke proses Anda, mukjizat akan terjadi tanpa banyak waktu, energi dan upaya dimasukkan ke dalam ini.

JeffO
sumber
4

Ini mungkin keras, tetapi jangan khawatir tentang memberikan umpan balik yang baik jika tidak ada yang baik untuk diukur.

Namun, di masa mendatang, saat pengembang Anda mulai meningkatkan kode mereka, saat itulah Anda ingin memberi mereka umpan balik yang baik. Anda ingin menunjukkan peningkatan dalam kode, dan Anda juga ingin menunjukkan manfaatnya kepada tim secara keseluruhan. Ketika Anda mulai melihat peningkatan, mereka juga akan, dan segala sesuatu harus perlahan mulai muncul.

Hal lain; mungkin ada udara defensif karena mereka merasa seolah-olah mereka tidak memiliki suara. Mengapa tidak membiarkan mereka meninjau kode masing-masing? Ajukan pertanyaan spesifik kepada mereka dan cobalah untuk melibatkan mereka. Seharusnya bukan Anda yang melawan mereka; itu harus menjadi upaya tim.

  1. Apa yang akan Anda ubah tentang kode ini jika Anda punya waktu?
  2. Bagaimana Anda meningkatkan area basis kode ini?

Tanyakan itu sekarang, dan tanyakan itu enam bulan dari sekarang. Ada pengalaman belajar di sini.

Poin utama - jangan memberikan pujian dalam hal kode di mana itu tidak dijamin, tetapi mengenali upaya dan pasti mengakui peningkatan. Cobalah membuat ulasan sebagai latihan tim alih-alih yang bermusuhan.

makan siangme317
sumber
1
"jangan khawatir tentang memberikan umpan balik yang baik jika tidak ada yang baik untuk diukur" Saya merasa sulit untuk percaya bahwa dia tidak bisa menemukan setidaknya sesuatu yang positif untuk dikatakan tentang kode yang ditulis oleh programmer profesional lainnya sambil tetap meningkatkan standar harapan semua orang kode. Ini tidak menjawab pertanyaan - itu hanya sebuah kontradiksi.
Aaron Hall
2
@ Harunall: "Kode Anda dapat berfungsi sebagai contoh yang baik bagaimana tidak menulis kode". Apakah itu cukup positif?
gnasher729
1
@ AaronHall Jika OP dapat menemukan sesuatu yang positif untuk dikatakan tentang kode yang ditulis oleh programmer profesional lainnya, maka dengan segala cara dia harus. Namun, jika tidak ada, maka tidak ada gunanya mencoba membuat sesuatu. Sebaliknya, OP harus fokus pada upaya pengembang dan pembelajaran, bukan kode itu sendiri.
lunchmeat317
4

Kualitas tanpa Ketegangan

Anda telah bertanya bagaimana Anda dapat menemukan hal-hal positif untuk dikatakan tentang kode, tetapi pertanyaan Anda yang sebenarnya adalah bagaimana Anda dapat menghindari "ketegangan di dalam tim [Anda]" sambil menangani "masalah kualitas yang serius."

Trik lama dalam mengapit ”berita buruk dalam kabar baik” mungkin menjadi bumerang. Pengembang cenderung melihatnya apa adanya: penemuan.

Masalah Top-Down Organisasi Anda

Atasan Anda menugaskan Anda untuk memastikan kualitas. Anda datang dengan daftar kriteria untuk kualitas kode. Sekarang, Anda ingin gagasan untuk penguatan positif diberikan kepada tim Anda.

Mengapa bertanya kepada kami apa yang perlu Anda lakukan untuk membuat tim Anda bahagia? Sudahkah Anda mempertimbangkan untuk bertanya kepada tim Anda?

Sepertinya Anda melakukan yang terbaik untuk menjadi baik. Masalahnya bukan bagaimana Anda mengirimkan pesan Anda. Masalahnya adalah komunikasi telah satu arah.

Membangun Budaya Kualitas

Budaya kualitas harus dipupuk untuk tumbuh dari bawah ke atas.

Biarkan bos Anda tahu bahwa Anda khawatir kualitasnya tidak meningkat cukup cepat, dan Anda ingin mencoba menerapkan beberapa saran dari The Harvard Business Review .

Temui tim Anda. Teladani perilaku yang ingin Anda lihat dalam tim Anda: kerendahan hati, rasa hormat, dan komitmen untuk meningkat.

Katakan sesuatu seperti: “Seperti yang Anda tahu, [rekan kerja] dan saya ditugaskan untuk memastikan kualitas kode, untuk mencegah masalah produksi yang kami alami baru-baru ini. Saya pribadi tidak berpikir saya telah memecahkan masalah ini. Saya pikir kesalahan terbesar saya adalah tidak melibatkan kalian lebih banyak di awal. [rekan kerja] dan saya masih bertanggung jawab kepada manajemen untuk kualitas kode, tetapi ke depan, kita semua dalam upaya kualitas bersama-sama. "

Dapatkan ide dari tim tentang kualitas kode. (Papan tulis akan membantu.) Pastikan persyaratan Anda sampai di sana pada akhirnya. Tawarkan wawasan Anda dengan cara yang sopan, dan ajukan pertanyaan bila perlu. Bersiaplah untuk terkejut tentang apa yang dirasakan tim Anda penting. Jadilah fleksibel, tanpa mengurangi kebutuhan bisnis.

(Jika Anda memiliki beberapa kode lama yang membuat Anda malu, Anda dapat menuliskannya, mendorong semua orang untuk berterus terang. Pada akhirnya, beri tahu orang-orang bahwa Anda yang menulisnya. Model reaksi dewasa yang Anda harapkan ketika orang lain menerima kritik konstruksi. )

Ulasan Kode Kolaboratif

Saya belum pernah bekerja di tempat seperti yang Anda gambarkan, di mana beberapa programmer senior meninjau semua kode dan membuat koreksi. Tidak heran orang-orang merespons seolah Anda adalah seorang guru yang membuat tanda merah di kertas mereka.

Jika Anda dapat menjual gagasan itu kepada manajemen, mulailah melakukan tinjauan kode rekan . Ini paling baik dilakukan dalam kelompok kecil, termasuk Anda atau pengembang yang bertanggung jawab lainnya. Pastikan semua orang diperlakukan dengan hormat.

Tujuan penting dari kode peer review adalah untuk memastikan kode tersebut dapat dipahami oleh semua anggota tim. Pertimbangkan contoh Anda dari nama fungsi yang tidak jelas: mendengar dari pengembang yang lebih junior bahwa mereka menemukan nama fungsi membingungkan dapat lebih mudah diterima daripada "koreksi" lain dari pengembang senior.

Kualitas adalah Perjalanan

Satu hal lain yang harus dilakukan adalah menghapus semua anggapan bahwa tinjauan kode adalah semacam hal yang lulus / gagal. Setiap orang harus berharap untuk melakukan beberapa pengeditan setelah peninjauan kode. (Dan tugasmu adalah memastikan itu terjadi.)

Tetapi jika itu tidak berhasil ...

Mari kita asumsikan Anda membuat beberapa langkah membangun budaya kualitas. Anda masih mungkin tidak mendapatkan semua orang di dalamnya. Jika seseorang masih tidak dengan program kualitas, sekarang masalahnya adalah mereka tidak cocok dengan tim, daripada ada masalah di antara kalian berdua. Jika mereka perlu diberhentikan dari tim, anggota tim lainnya akan lebih memahami alasannya.

Tim Grant
sumber
Hati-hati dengan ulasan kode peer. Kami melakukan itu untuk sementara waktu, sampai kami menyadari bahwa seorang junior dev melakukan review untuk seorang junior dev lainnya, dan mengizinkan kode yang seharusnya tidak pernah lulus. Dua senior di tim sekarang melakukan review.
Gustav Bertram
4

Maaf untuk jawaban panjang lain, tapi saya tidak berpikir yang lain telah sepenuhnya membahas elemen manusia dari masalah ini.

terkadang bahkan hanya bertanya mengapa sesuatu diterapkan dengan cara tertentu. Ketika saya pikir itu buruk, saya menunjukkan bahwa saya akan mengembangkannya dengan cara lain.

Masalah dengan "Mengapa Anda menerapkannya dengan cara ini?" adalah bahwa Anda segera membuat pengembang bersikap defensif. Pertanyaan itu sendiri dapat menyiratkan segala macam hal tergantung pada konteks: Apakah Anda terlalu bodoh untuk memikirkan solusi yang lebih baik? Apakah ini yang terbaik yang dapat Anda lakukan? Apakah Anda mencoba untuk merusak proyek ini? Apakah Anda peduli dengan kualitas pekerjaan Anda? dll. Menanyakan 'mengapa' kode itu dikembangkan dengan cara tertentu hanya akan bersifat konfrontatif, dan itu merongrong niat pedagogis apa pun yang mungkin dimiliki komentar Anda.

Demikian pula, "Saya akan melakukan ini secara berbeda ..." juga konfrontatif, karena pemikiran langsung yang akan dimiliki pengembang adalah " Yah saya melakukannya dengan cara ini ... Anda punya masalah dengan itu? " Dan lagi, itu lebih konfrontatif daripada itu perlu dan mengubah diskusi dari memperbaiki kode.

Contoh:

Alih-alih bertanya "Mengapa Anda memilih untuk tidak menggunakan variabel konstan untuk nilai ini?", Cukup nyatakan "Nilai hard-kode ini harus diganti dengan konstanta XYZdalam file Constants.h" Mengajukan pertanyaan menyarankan bahwa pengembang secara aktif memilih untuk tidak menggunakan sudah didefinisikan konstan, tetapi sangat mungkin bahwa mereka bahkan tidak tahu itu ada. Dengan basis kode yang cukup besar, akan ada banyak hal yang tidak diketahui oleh setiap pengembang. Ini hanyalah kesempatan belajar yang baik bagi pengembang untuk berkenalan dengan konstanta proyek.

Ada garis yang bagus untuk berjalan dengan ulasan kode: Anda tidak perlu menutupi semua yang Anda katakan, Anda tidak perlu menyamping berita buruk dengan berita baik, dan Anda tidak perlu melunakkan pukulannya. Bisa jadi budaya tempat saya bekerja, tetapi rekan kerja saya dan saya tidak pernah saling memuji dalam ulasan kode - bagian dari kode yang kami kembangkan yang bebas cacat sudah cukup sebagai pujian. Yang perlu Anda lakukan adalah mengidentifikasi cacat yang Anda lihat, dan mungkin memberikan alasan mengapa (alasan itu kurang berguna ketika jelas / sederhana).

Baik:

  • "Nama variabel ini perlu diubah agar sesuai dengan standar pengkodean kami."

  • "Huruf 'b' dalam nama fungsi ini perlu dikapitalisasi agar menjadi PascalCase."

  • "Kode fungsi ini tidak indentasi dengan benar."

  • "Kode ini adalah duplikat dari kode yang ditemukan ABC::XYZ(), dan harus menggunakan fungsi itu sebagai gantinya."

  • "Anda harus menggunakan coba-dengan-sumber daya sehingga hal-hal dijamin akan ditutup dengan benar dalam fungsi ini, bahkan jika kesalahan terjadi." [Saya hanya menambahkan tautan di sini sehingga pengguna non-java akan tahu apa artinya mencoba-dengan-sumber daya]

  • "Fungsi ini perlu direaktor ulang untuk memenuhi standar kompleksitas n-path kami."

Buruk:

  • "Saya pikir Anda dapat meningkatkan kode ini dengan mengubah nama variabel agar sesuai dengan standar kami"

  • " Mungkin akan lebih baik menggunakan try-with-resource untuk menutup dengan benar hal-hal dalam fungsi ini"

  • " Mungkin diinginkan untuk melihat kembali semua kondisi dalam fungsi ini. Kompleksitas N-path-nya melebihi kompleksitas maksimum yang diizinkan standar kami."

  • "Mengapa indentasi di sini 2 spasi, bukan 4 standar kita?"

  • "Mengapa kamu menulis fungsi yang melanggar standar kompleksitas n-path kami?"

Dalam pernyataan yang buruk, bagian yang dicetak miring adalah hal-hal yang biasa digunakan orang ketika mereka ingin melunakkan pukulan, tetapi yang benar-benar dilakukannya dalam tinjauan kode adalah membuat Anda terdengar tidak yakin akan diri sendiri. Jika Anda, peninjau, tidak yakin tentang cara meningkatkan kode, lalu mengapa orang harus mendengarkan Anda?

Pernyataan 'baik' itu tumpul, tetapi mereka tidak menuduh pengembang, mereka tidak menyerang pengembang, mereka tidak konfrontatif, dan mereka tidak mempertanyakan mengapa cacat itu ada. Itu ada; inilah solusinya. Mereka tentu tidak se-konfrontasional dengan pertanyaan 'mengapa' terakhir.

Shaz
sumber
1
Banyak contoh yang Anda berikan akan dideteksi oleh analisis statis. Dalam pengalaman saya, hal-hal yang muncul dalam ulasan kode sering kali lebih subyektif dan berpendapat: "Saya akan memanggil XY sebagai gantinya karena saya pikir itu lebih mencerminkan perilaku". Dalam organisasi kami, pencipta PR kemudian dapat menggunakan penilaiannya sendiri dan baik mengubah nama atau membiarkannya apa adanya.
Muton
@Muton Saya setuju dengan Anda tentang analisis statis. Kami memiliki cek itu otomatis di proyek yang saya kerjakan. Kami juga memiliki alat yang mengotomatiskan pemformatan kode sehingga sebagian besar masalah gaya pengkodean tidak pernah (atau jarang) muncul. Tetapi OP secara khusus menyebutkan bahwa basis kode mereka berantakan, jadi saya membayangkan ini adalah jenis masalah yang mereka hadapi dalam ulasan, dan saya pikir contoh sederhana ini tetap relevan dengan pembaruan OP yang dibuat untuk menampilkan contoh ulasan.
Shaz
3

Masalah yang Anda lihat adalah: Pengembang tidak senang bahwa kode mereka dikritik. Tapi bukan itu masalahnya. Masalahnya adalah bahwa kode mereka tidak baik (dengan asumsi jelas bahwa ulasan kode Anda baik).

Jika pengembang tidak suka kode mereka menarik kritik, maka solusinya mudah: Tulis kode yang lebih baik. Anda mengatakan Anda memiliki masalah serius dengan kualitas kode; itu berarti kode yang lebih baik diperlukan.

"Mengapa metode ini harus memiliki nama yang masuk akal, aku tahu apa fungsinya?" adalah sesuatu yang saya temukan sangat mengganggu. Dia tahu apa fungsinya, atau setidaknya dia mengatakannya, tapi aku tidak. Metode apa pun seharusnya tidak hanya memiliki nama yang masuk akal, itu harus memiliki nama yang segera membuatnya jelas bagi pembaca kode apa yang dilakukannya. Anda mungkin ingin pergi ke situs web Apple dan mencari video WWDC tentang perubahan dari Swift 2 ke Swift 3 - sejumlah besar perubahan dibuat hanya untuk membuat semuanya lebih mudah dibaca. Mungkin video semacam itu dapat meyakinkan pengembang Anda bahwa pengembang yang jauh lebih pintar daripada mereka menemukan nama metode intuitif menjadi sangat, sangat penting.

Item lain yang mengganggu adalah kolega Anda yang mengatakan "dia mengatakan kepada saya sendiri, bahwa setelah laporan bug diajukan oleh pelanggan, bahwa dia tahu bug itu, tetapi takut bahwa pengembang akan marah padanya karena menunjukkannya". Ada kemungkinan ada beberapa kesalahpahaman, tetapi jika pengembang marah jika Anda menunjukkan bug kepada mereka, itu buruk.

gnasher729
sumber
+1 Dev mungkin tahu apa fungsi sub-optimalnya, tetapi apa yang terjadi ketika dia naik bus?
Mawg
3

Dengan hormat saya tidak akan setuju dengan metode tinjauan kode yang telah Anda jelaskan. Dua staf pergi ke 'ruang tertutup' dan keluar dengan kritik melembagakan gagasan permusuhan yang harus dihindari .

Membuat ulasan kode pengalaman positif sejauh mungkin adalah penting untuk membuatnya sukses. Langkah pertama adalah menghapus gagasan peninjauan yang berlawanan. Buatlah mingguan atau dua mingguan kelompok acara; memastikan semua orang tahu mereka dipersilakan untuk berpartisipasi. Pesan pizza atau sandwich atau apa pun. Bahkan jangan menyebutnya 'tinjauan kode' jika itu membangkitkan konotasi negatif. Temukan sesuatu untuk dirayakan, untuk didorong, untuk dibagikan - bahkan jika itu tidak lebih dari kemajuan melalui sprint atau iterasi saat ini. Bergantian menugaskan kepemimpinan atas ulasan.

Jadikan proses sebagai upaya untuk melayani produk dan orang-orang. Jika mereka dilakukan secara konstruktif, positif, di mana teknik atau pola yang baik dibagikan dan didorong sama seperti yang miskin tidak dianjurkan - semua orang mendapat manfaat. Semua orang dilatih di depan umum. Jika Anda memiliki pemrogram bermasalah yang tidak "mendapatkannya", mereka harus ditangani secara pribadi dan terpisah - jangan pernah di depan kelompok yang lebih luas. Itu terpisah dari ulasan kode.

Jika Anda kesulitan menemukan sesuatu yang "baik" untuk dibicarakan, itu bagi saya menimbulkan pertanyaan: Jika kemajuan sedang dibuat dalam proyek, dan orang-orang bekerja, kemajuan itu sendiri adalah sesuatu untuk dirayakan. Jika Anda benar-benar menemukan tidak ada yang baik, itu menyiratkan semua yang telah dilakukan sejak ulasan terakhir buruk atau tidak lebih baik dari netral . Apakah benar hal itu merupakan masalahnya?

Adapun rinciannya, standar umum sangat penting. Beri setiap orang saham apa yang sedang dilakukan. Dan memungkinkan teknik baru yang lebih baik untuk diintegrasikan ke dalam basis kode Anda. Kegagalan melakukan hal itu menjamin kebiasaan lama tidak pernah dihapuskan dengan kedok "Kami selalu melakukannya dengan cara itu."

Maksud dari semua ini adalah bahwa tinjauan kode diberikan sedikit kesalahan karena mereka diimplementasikan dengan buruk, digunakan sebagai palu untuk meremehkan programmer yang kurang berpengalaman atau kurang terampil, dan itu tidak berguna bagi siapa pun. Manajer yang menggunakannya dengan cara ini juga tidak cenderung menjadi manajer yang sangat efektif. Ulasan kode yang baik dimana setiap orang adalah peserta melayani semua orang - produk, karyawan, dan perusahaan.

Permintaan maaf untuk panjang posting, tetapi setelah berada dalam grup yang sebagian besar ulasannya diabaikan, kode itu menuntun pada warisan kode yang tidak dapat dipertahankan dan hanya sejumlah kecil pengembang yang dapat melakukannya meskipun diizinkan untuk membuat perubahan pada basis kode yang lama. Itu adalah jalan yang saya pilih untuk tidak dilintasi lagi.

David W
sumber
+1 untuk peninjauan kode secara langsung alih-alih secara elektronik - yang akan menghilangkan kritik
alexanderbird
3

Paradoks dari kode yang baik adalah bahwa itu tidak menonjol sama sekali, dan sepertinya itu sangat mudah dan mudah untuk ditulis. Saya sangat suka contoh pemain biliar dari jawaban ini . Dalam ulasan kode, itu membuatnya sangat mudah untuk ditutup-tutupi, kecuali jika itu merupakan refactor dari kode yang buruk.

Saya membuat titik untuk mencarinya. Jika saya meninjau kode dan saya telah membaca file yang sangat mudah dibaca sehingga tampak mudah untuk ditulis, saya hanya memasukkan "Saya suka bagaimana metode di sini pendek dan bersih" atau apa pun yang sesuai .

Anda juga harus memimpin dengan memberi contoh. Anda harus mendesak agar kode Anda ditinjau juga, dan Anda harus memodelkan bagaimana Anda ingin tim Anda berperilaku ketika menerima koreksi. Yang paling penting, Anda harus dengan tulus berterima kasih kepada orang-orang atas umpan baliknya. Ini membuat lebih banyak perbedaan daripada umpan balik sepihak yang dapat Anda berikan.

Karl Bielefeldt
sumber
1

Kedengarannya seperti masalah sebenarnya adalah hanya dua orang yang melakukan semua tinjauan kode, yang hanya Anda yang menganggapnya serius, yang telah menyebabkan Anda berada dalam situasi yang tidak menguntungkan dengan banyak tanggung jawab dan banyak tekanan dari anggota tim lainnya.

Cara yang lebih baik adalah dengan mendistribusikan tanggung jawab melakukan tinjauan kode atas tim, dan mintalah setiap orang mengambil bagian dalam melakukannya, misalnya dengan membiarkan pengembang memilih siapa yang akan meninjau kode mereka. Ini adalah sesuatu yang harus didukung oleh alat peninjau kode Anda.

Halo selamat tinggal
sumber
Tidak yakin mengapa ini dibatalkan (downvotes tanpa komentar sangat konyol di utas tentang tinjauan kode yang baik). Setiap orang yang meninjau harus menjadi prosedur standar. Pada papan Kanban, akan ada kolom untuk ulasan kode, dan siapa pun dalam tim mengambil item berikutnya harus melakukan review (dengan peringatan; pemula tidak boleh mengambil ulasan untuk sementara waktu, dan kemudian harus mulai dari mereka yang membutuhkan sedikit pengetahuan domain). Pada papan scrum, dasarnya serupa: bekerja dari kanan ke kiri.
Dewi Morgan
1

Saya telah melihat ini terjadi secara langsung dan ingin mengingatkan Anda jauh dari jawaban yang menantang kecerdasan emosional - mereka dapat membunuh seluruh tim. Kecuali jika Anda ingin merekrut, melatih, dan menormalkan pengembang baru setiap tahun, menciptakan hubungan positif dengan rekan kerja Anda sangat penting. Lagi pula, bukankah seluruh titik melakukan tinjauan ini untuk meningkatkan kualitas kode dan menumbuhkan budaya di mana kualitas kode lebih tinggi di tempat pertama? Saya berpendapat bahwa memang bagian dari pekerjaan Anda untuk memberikan penguatan positif sebagai cara mengintegrasikan sistem peninjauan kode ini ke dalam budaya tim.

Ngomong-ngomong, sepertinya Anda perlu meninjau sistem Tinjauan Kode Anda. Saat ini, dari semua itu, segala sesuatu dalam proses peninjauan Anda adalah, atau dapat diartikan sebagai, subyektif daripada objektif. Sangat mudah untuk mendapatkan perasaan terluka ketika rasanya seperti seseorang hanya mengambil kode Anda karena mereka tidak menyukainya, daripada mereka memiliki alasan bahwa mereka dapat mengutip ketika ada sesuatu yang tidak sesuai dengan pedoman. Dengan cara ini, mudah untuk melacak dan 'merayakan' peningkatan positif dalam kualitas kode (sehubungan dengan sistem ulasan Anda) dengan cara apa pun yang sesuai untuk budaya kantor Anda.

Pimpinan Teknis perlu berkumpul di luar sesi peninjauan dan mengeluarkan Daftar Periksa Pedoman / Pengkodean Kode dan harus dipatuhi dan dirujuk selama peninjauan. Ini harus menjadi dokumen hidup yang dapat diperbarui dan disempurnakan saat proses berkembang. Pertemuan Pimpinan Teknologi ini juga harus dilakukan ketika diskusi 'Cara kami selalu melakukan hal-hal' vs. 'Cara-cara baru dan lebih baik dalam melakukan sesuatu' harus terjadi, tanpa pengembang ditinjau merasa bahwa ketidaksepakatan adalah hasil dari kode mereka. Setelah pedoman awal telah lebih atau kurang dihaluskan, cara lain untuk secara positif memperkuat pengembang adalah meminta umpan balik mereka dan kemudian benar-benar mengambil tindakan atasnya dan berevolusi proses sebagai tim, ini keajaiban untuk membawa mereka dengan cepat untuk mulai meninjau kode untuk onboard sehingga Anda tidak terjebak melakukan review sampai Anda pensiun juga.

navigator_
sumber
1

Tempat ...

Programmer adalah pemecah masalah. Kami dikondisikan untuk mulai men-debug ketika dihadapkan dengan masalah atau kesalahan.

Kode adalah media format garis besar. Beralih ke narasi format paragraf untuk umpan balik memperkenalkan putuskan yang harus diterjemahkan. Mau tidak mau sesuatu hilang atau disalahpahami. Tidak dapat dihindari, resensi buku tidak berbicara dalam bahasa pemrogram.

Pesan kesalahan yang dihasilkan komputer jarang dipertanyakan dan tidak pernah dianggap sebagai penghinaan pribadi.

Item ulasan kode adalah pesan kesalahan yang dibuat manusia. Mereka dimaksudkan untuk menangkap kasus-kasus tepi yang dilewatkan oleh pemrogram dan alat otomatis serta efek samping yang tidak terhindarkan pada program dan data lainnya.

Kesimpulan ...

Dengan demikian, semakin banyak item ulasan kode dapat dimasukkan ke dalam alat otomatis, semakin baik mereka akan diterima.

Peringatannya adalah bahwa, untuk tetap tidak dipertanyakan, alat-alat seperti itu harus terus diperbarui, biasanya setiap hari atau setiap minggu, agar sesuai dengan setiap perubahan pada prosedur, standar, dan praktik. Ketika manajer atau tim pengembang memutuskan untuk melakukan perubahan, alat harus diubah untuk menegakkannya. Sebagian besar pekerjaan peninjau kode kemudian menjadi mempertahankan aturan dalam alat.

Umpan Balik Ulasan Contoh Kode ...

Menulis ulang contoh yang diberikan menggabungkan teknik ini:

  • Subyek:

    • Library \ ACME \ ExtractOrderMail Class.
  • Masalah prinsip ...

    • TempFilesToDelete bersifat statis
      • Panggilan berikutnya ke GetMails membuat pengecualian karena file ditambahkan ke dalamnya tetapi tidak pernah dihapus setelah dihapus. Meskipun hanya satu panggilan sekarang, kinerja dapat ditingkatkan di masa depan dengan paralelisme.
      • TempFilesToDelete sebagai variabel instan akan memungkinkan banyak objek untuk digunakan secara paralel.
  • Masalah sekunder ...
    • GetErrorMailBody memiliki parameter pengecualian
      • Karena tidak membuang pengecualian itu sendiri, dan hanya meneruskan ini ke ToString, apakah itu perlu?
    • Simpan Nama Kirim
      • Email mungkin atau mungkin tidak digunakan untuk melaporkan ini di masa depan dan kode ini berisi logika umum untuk menyimpan salinan yang persisten dan melaporkan kesalahan apa pun. Nama yang lebih umum akan memungkinkan perubahan yang diantisipasi tersebut tanpa mengubah metode dependen. Salah satu kemungkinan adalah StoreAndReport.
    • Mengomentari kode lama
      • Meninggalkan baris yang dikomentari dan ditandai OBSOLETE bisa sangat membantu dalam debugging, tetapi "dinding komentar" juga dapat mengaburkan kesalahan dalam kode yang berdekatan. Kami masih memilikinya di Subversion. Mungkin hanya komentar yang merujuk persis di mana di Subversion?
DocSalvager
sumber
0

Jika Anda tidak memiliki sesuatu yang baik untuk dikatakan tentang kode yang ada (yang kedengarannya dapat dimengerti), cobalah untuk melibatkan pengembang dalam memperbaikinya.

Di tambalan untuk perubahan fungsional atau perbaikan bug, tidak akan membantu jika ada perubahan lain (penggantian nama, refactoring, apa pun), digabungkan ke tambalan yang sama. Itu harus membuat perubahan yang memperbaiki bug atau menambahkan fitur, tidak ada yang lain.

Dimana penggantian nama, refactoring dan perubahan lain yang ditunjukkan, melakukannya di patch terpisah, sebelum atau sesudah.

  • Ini sangat jelek, tapi saya kira itu konsisten dengan semua kode jahat kami yang lain. Mari kita bersihkan nanti (dalam tambalan dengan, idealnya, tidak ada perubahan fungsional).

    Ini juga membantu jika Anda bergabung dalam refactoring, dan biarkan mereka meninjau kode Anda . Ini memberi Anda kesempatan untuk mengatakan mengapa Anda berpikir sesuatu itu baik, bukan buruk, dan juga untuk menunjukkan contoh menerima kritik yang membangun dengan baik.

Jika Anda perlu menambahkan unit test ke fitur baru, baiklah, mereka masuk di patch utama. Tetapi jika Anda memperbaiki bug, tes harus dilakukan di patch sebelumnya dan tidak lulus sehingga Anda dapat menunjukkan perbaikan yang benar-benar memperbaiki bug.

Idealnya rencana (untuk bagaimana Anda dapat menguji suatu perbaikan, atau apa yang harus diperbaiki dan kapan) harus didiskusikan sebelum pekerjaan dilakukan, sehingga mereka belum melakukan banyak upaya sebelum menemukan bahwa Anda memiliki masalah dengan itu.

Jika Anda menemukan kode tidak sesuai dengan input yang Anda pikir seharusnya, itu mungkin juga merupakan ketidakcocokan dalam apa yang dilihat pengembang sebagai input yang masuk akal. Setujuilah sebelumnya juga, jika mungkin. Setidaknya berdiskusi tentang apa yang harus dapat diatasi dan seberapa buruk itu bisa dibiarkan gagal.

Tak berguna
sumber
Apakah dalam tambalan yang terpisah atau tidak, kebijakan jendela yang rusak perlu dipasang dengan kode lawas, apakah kebijakan itu adalah "jangan memperbaiki jendela yang rusak" atau "hanya di [metode / kelas / file?] Yang disentuh oleh tambalan saat ini ". Dalam pengalaman saya, mencegah pengembang dari memperbaiki jendela yang rusak adalah racun bagi semangat tim.
Dewi Morgan
1
Benar, tetapi memaksa mereka untuk memperbaiki setiap jendela yang rusak dalam sebuah modul sebelum perubahan satu-barisnya melewati tinjauan sama-sama beracun.
berguna
Sepakat! Suatu kebijakan yang telah diterapkan oleh tim, alih-alih dipaksakan secara eksternal, adalah satu-satunya jenis yang dapat berhasil, saya rasa.
Dewi Morgan
0

Saya pikir itu adalah kesalahan untuk mengasumsikan ada solusi teknis atau mudah untuk: "rekan kerja saya marah ketika saya menilai pekerjaan mereka menurut standar saya, dan memiliki kekuatan untuk menegakkannya".

Ingat ulasan kode tidak hanya diskusi tentang apa yang baik atau buruk. Mereka secara implisit adalah "siapa yang menggunakan tongkat", "" yang membuat keputusan "dan karenanya - paling berbahaya - peringkat.

Jika Anda tidak membahas poin-poin itu, melakukan tinjauan kode dengan cara yang tidak memiliki masalah yang Anda temui akan sulit. Ada beberapa saran bagus di sini tentang cara melakukannya, tetapi Anda telah menetapkan tugas yang sulit bagi diri Anda sendiri.

Jika Anda benar, tanpa ruang gerak, menunjukkannya adalah serangan: seseorang mengacau. Jika tidak: MBA Anda yang lain yang tidak mendapatkannya. Either way, Anda akan menjadi orang jahat.

Namun, jika apa yang dilihat ulasan dan mengapa , jelas dan disepakati, Anda memiliki peluang yang layak untuk tidak menjadi penjahat. Anda bukan penjaga gerbang, hanya seorang korektor. Mendapatkan persetujuan ini adalah masalah yang sulit untuk dipecahkan [1]. Saya ingin memberi Anda nasihat, tetapi saya belum memahami itu sendiri ...

[1] Itu tidak terpecahkan hanya karena orang mengatakan "ok", atau berhenti bertengkar tentang hal itu. Tidak seorang pun ingin menjadi orang yang mengatakan X tidak praktis untuk {kecerdasan, pengalaman, tenaga-manusia, tenggat waktu, dll} tetapi itu tidak berarti ketika itu benar-benar datang ke beberapa contoh spesifik melakukan X ...

drjpizzle
sumber
-1

Ulasan kode harus saling menguntungkan. Semua orang mengkritik semua orang. Biarkan moto menjadi "Jangan marah, balas." Semua orang membuat kesalahan dan sekali orang memberi tahu cara memperbaiki kode mereka, mereka akan mengerti bahwa ini hanya prosedur biasa dan bukan serangan terhadap mereka.

Melihat kode orang lain itu mendidik. Memahami kode ke titik di mana Anda dapat menunjukkan titik lemahnya dan harus menjelaskan bahwa kelemahan dapat mengajari Anda a banyak hal!

Ada sedikit ruang untuk pujian dalam ulasan kode, karena intinya adalah menemukan kekurangan. Namun, Anda dapat mengumpulkan pujian untuk perbaikan . Baik ketepatan waktu dan kerapian dapat dipuji.

Stig Hemmer
sumber
Tidak yakin mengapa ini dibatalkan (downvotes tanpa komentar sangat konyol di utas tentang tinjauan kode yang baik). Setiap orang yang meninjau harus menjadi prosedur standar. Pada papan Kanban, akan ada kolom untuk ulasan kode, dan siapa pun dalam tim mengambil item berikutnya harus melakukan review (dengan peringatan; pemula tidak boleh mengambil ulasan untuk sementara waktu, dan kemudian harus mulai dari mereka yang membutuhkan sedikit pengetahuan domain). Pada papan scrum, dasarnya serupa: bekerja dari kanan ke kiri.
Dewi Morgan
2
@DewiMorgan Saya tidak setuju dengan "pemula tidak boleh mengambil ulasan untuk sementara waktu". Pemula yang melakukan review adalah cara terbaik bagi mereka untuk mendapatkan keakraban dengan basis kode. Yang mengatakan, mereka seharusnya bukan satu - satunya peninjau! Dan yang mengatakan, saya dalam hal apapun juga waspada hanya memiliki satu resensi sebagian besar waktu.
Disillusioned