“Suatu ketika kami melakukan peninjauan kode, menulis komentar melalui pos, menunjukkan nomor baris. Itu sangat lucu. Dari pro: tidak ada yang melihat apa-apa di diff, melihat di IDE. Tapi ada juga minus: setelah beberapa penggabungan, nomor baris berubah. "
Alexander Makarov, Yii
“Perusahaan kami memiliki konsep yang menarik - permintaan kursi . Ini adalah saat, dalam kerangka satu kantor, seorang pengembang mendatangi Anda di kursi dan berkata: 'Lihat, ini lebih cepat daripada membuat permintaan tarik.'
Anton Morev, WormSoft
Baru-baru ini, ada rekaman publik dari podcast SDCast tentang tinjauan kode di YouTube. Kami telah memilih dan menguraikan yang paling menarik dari masalah ini.
Versi audio lengkap di Spotify, Ya Music dan sebagai file untuk diunduh
Versi video lengkap di Youtube
Jangan perlakukan review kode seperti memeriksa sesuatu atau mencari bug
Sergey Zhuk, Skyeng: Saya percaya bahwa tujuan mendasar dari tinjauan kode adalah untuk berbagi pengetahuan dalam tim dan menemukan solusi terbaik. Tim meninjau perubahan yang diusulkan - dan pengetahuan tentang domain didistribusikan secara merata di antara anggotanya. Penulis solusi memahami bagaimana kode yang menurutnya sempurna sebenarnya dapat ditingkatkan.
Oleh karena itu, peninjauan kode bukanlah sesuatu yang harus dihindari atau dilakukan lebih cepat. Ini adalah investasi dalam bisnis dan tim: kode menjadi lebih baik, tim menjadi lebih baik. Ya, awalnya saya tidak suka ketika permintaan dibungkus (dan siapa yang mau).
Tetapi kemudian saya mulai melihatnya sebagai proses pembelajaran, bersama dengan membaca buku atau berpartisipasi dalam konferensi.
Saya merasakan ini plus diri saya sendiri, sebagai pengembang, saya tumbuh dengan pendekatan ini.
Tapi ada nuansa. Tentunya banyak dari Anda telah menemukan permintaan untuk seribu baris, di mana pemfaktoran ulang, dan fitur, dan beberapa perubahan kecil dicampur. Dengan mencampurkan berbagai hal dalam satu permintaan, kami mempersulit berbagi pengetahuan dan kehidupan pengulas: akan lebih sulit baginya untuk menilai apakah perilaku sistem telah berubah, apakah persyaratan bisnis telah terpenuhi, apakah masalah secara keseluruhan telah diselesaikan - dan apakah solusinya berhasil?
Kami memperhatikan momen ini di tim kami dan memperkenalkan aturan: dalam satu permintaan tarik, jangan mengganggu perubahan yang bersifat berbeda. Anda mengirim refactoring secara terpisah, fitur terpisah dan perubahan kecil secara terpisah.
Laporan Sergey tentang praktik timnya. Versi teks ada di sini .
Permintaan ini biasanya ditinjau dengan cepat dan mudah, dan kemungkinan besar akan menerima umpan balik yang berkualitas. Dan di pihak pengelola, ada kelebihan: jika Anda suka refactoring, dan fitur dengan bug, maka Anda bisa menggabungkan refactoring secara terpisah.
Alexander Makarov: Saya setuju bahwa Anda tidak boleh mencoba menghabiskan waktu sesedikit mungkin untuk ulasan kode. Dibuka, seperti tanda kurung tidak masalah, kode ini melakukan sesuatu - tidak berfungsi seperti itu. Jika peninjau tidak dapat sepenuhnya menjamin kode, maka peninjauan kode belum dilakukan.
Oleh karena itu, kami sekarang telah mulai menyusun sejumlah besar pedoman untuk diri kami sendiri, dan salah satunya berbicara tentang tinjauan kode.
Bagian dari pedoman tim Yii.
Tetapi untuk tesis tentang permintaan tarik kecil individu: Saya memiliki situasi ketika petunjuk datang dan memperkenalkan sesuatu seperti itu. Tim itu bermusuhan. Bagaimana Anda mendapatkannya?
Sergey Zhuk: Ada tim paralel yang kami gunakan untuk berinteraksi dan menggunakan API mereka. Mereka membuat banyak fitur dalam satu bulan, kami hanya melakukan sedikit. Artinya, awalnya kami tidak fokus pada penyampaian fitur, tetapi pada kualitas dengan pendekatan ini. Dan kami setuju dengan bisnis bahwa kami akan merilisnya lebih lambat, tetapi kami berusaha untuk tidak merusak apa pun. Sebulan kemudian, salah satu tetangga rusak, lalu tetangga lainnya. Tapi kami tidak. Dan pada contoh ini, semua orang memahami segalanya.
Konstantin Burkalev, SDCast:Saya memiliki proses implementasi tinjauan kode secara umum - dan itu tidak mudah, meskipun semua orang tampaknya mengerti bahwa itu bagus, ya. Anda berkata, "Teman-teman, mari bergabung melalui permintaan tarik." Mereka berkata kepada Anda: "Ya, ada fitur kecil."
Prinsip di sini benar-benar bekerja dengan baik bahwa ketika ada sesuatu yang rusak, Anda berkata: "Tetapi jika Anda membuat permintaan, kami akan melihat dan itu tidak sampai ke sana." Idenya adalah agar orang memahami kesalahan dari pengalaman mereka sendiri. Mencoba memaksakan tidak selalu berhasil.
Seberapa cepat meninjau
Selama siaran, pemungutan suara terjadi di antara penonton. Ini salah satunya.
Konstantin Burkalev: Juni sering kali seperti ini: "Nah, Anda memperhatikan permintaan saya, apakah tidak apa-apa di sana? Saya menulisnya, mengepalkan tangan dan menunggu ”.
Dan saya memiliki tugas saya sendiri, saya hanya bisa sampai di sana pada malam hari atau saya tidak tahu sama sekali ...
Sergey Zhuk: Di tim kami, peninjauan selalu menjadi prioritas. Oleh karena itu, ada peraturan - ketika permintaan tarik tiba, Anda menyelesaikan apa yang Anda lakukan sekarang, agar tidak kehilangan konteks, dan pergi untuk meninjau. Karena apa yang akan kamu lakukan masih dalam proses. Dan tugas itu telah selesai.
Kode sudah ditulis, tinggal melihat, menggabungkan dan mengunggahnya ke produk.
Artinya, saya menyelesaikan sesuatu sendiri, beralih, dengan cepat melihat dan terus bekerja. Mungkin, 3 kali sehari saya terganggu dari tugas saya untuk ditinjau. Tapi, sekali lagi, Anda harus memahami, di tim saya semuanya dibagi menjadi permintaan tarik kecil, masing-masing 200-300 baris kode. Oleh karena itu, waktu minimal dihabiskan.
"Siapa yang mengulas kurang penting daripada siapa"
Konstantin Burkalev: Ini menimbulkan pertanyaan - siapa yang harus meninjau. Timbal saja? Hanya senor? Atau dapat dan harus diberikan kepada seseorang yang berkompetensi lebih rendah, hanya agar orang tersebut mencoba untuk berkembang?
Dan ketika ditanya apa yang harus diulas, orang-orang menjawab seperti ini.
Alexander Makarov: Jika Anda memiliki banyak orang di tim Anda, dan keunggulan telah menciptakan hambatan dari dirinya sendiri, itu memperlambat seluruh tim dan akibatnya membuat tim jauh lebih buruk. Sebagai pemimpin, ketika saya bekerja di Skyeng, saya memiliki 15 permintaan tarik sehari pada puncaknya, dan bukan yang terkecil. Saya menyisihkan waktu untuk mereview pagi dan sore hari.
Semua orang perlu ditinjau. Kecuali, mungkin, cerita di mana "api, semuanya terbakar" - tidak akan bertambah buruk di sana.
Saya mengacaukannya, tidak apa-apa - meskipun saya telah menggunakan proyek yang sama selama bertahun-tahun. Oleh karena itu, sekarang saya mencoba mengundang sebanyak mungkin orang untuk menonton permintaan saya. Ini bagus dan seharusnya begitu.
Ini masalah lain apakah setiap orang harus mempercayai ulasan tersebut. Saya memiliki orang-orang yang bisa menganggapnya hebat, tetapi mereka memiliki masalah besar dengan fokus: dan, katakanlah, seseorang menghabiskan 6 jam untuk ulasan. Saya harus mengajari orang bagaimana mengatur waktu mereka.
Jika kita berbicara tentang perusahaan komersial, saya mendukung untuk mengidentifikasi siapa yang memiliki tugas ini dan siapa yang dapat meninjau sesuka hati - dan berapa banyak waktu per hari yang dapat Anda gunakan untuk meninjau, tergantung pada status ini.
Anton Morev:Seperti yang saya lihat, ada proses yang berbeda. Jika kami melakukan beberapa fitur yang perlu dirilis dalam waktu singkat, kami tidak punya waktu untuk membiarkan Jun melihat kodenya. Tetapi jika kita melakukan beberapa jenis produk internal atau tenggat waktu tidak tinggi, ya, itu praktik yang bagus untuk membiarkan June meninjau ulang apa yang dilakukan oleh pemimpin atau pengembang senior. Jadi keluarga Jun akan lebih memahami apa yang terjadi dalam proyek secara keseluruhan dan bagaimana mekanisme pengambilan keputusan bekerja.
"Ini langsung ditolak"
Sergey Zhuk: Skyeng telah menerapkan pemeriksaan untuk pesan komit: harus ada nomor tugas di JIRA, jika tidak, permintaan tarik tidak dapat digabungkan. Terkadang, Anda membuka kode, Anda tidak mengerti apa yang ada di sana - Anda membuka tugas dan Anda dapat memahami sesuatu.
Jika tidak, pada awalnya itu sulit, lalu kami memutuskan untuk menolaknya hanya jika tugas itu benar-benar salah, atau tim secara arsitektur tidak setuju. Jadi: kami memberikan persetujuan, menggabungkan, menulis komentar - dan jika pembuat permintaan tarik ingin, dia akan kembali dan memperbaikinya. Di sana, dia akan memperbaiki kekasaran kecil atau menggunakan semacam pola. Apa praktik penolakan Anda?
Alexander Makarov: Kriteria tersebut sepenuhnya sesuai dengan kriteria Anda - jika tugas tidak dilakukan dengan baik, tentu saja, Anda harus menyelesaikannya. Bahkan jika kodenya terlihat bagus dan secara arsitektural semuanya keren.
, : , .
Misalnya, kita berkata: "Teman-teman, ayo kita tes". Tentu saja ada pengecualian, tetapi tes itu sangat penting. Jelas dari mereka apakah orang tersebut memahami tugas dengan benar: sekali lagi, jika dia menguji kelas dan metode, dan bukan kasus penggunaan, ini sudah mencurigakan. Kami sekarang telah mengacaukan infeksi , ini adalah pengujian mutasi. Tulza meninggalkan pengujian yang sama, tetapi memodifikasi kode dan menjalankannya. Dan jika kode yang diubah lulus dengan tes yang sama, maka bagian dari kode tidak diperlukan - Anda bisa mengambilnya, menghapusnya, tidak ada yang berubah.
Sedikit di belakang panggung.
Juga, tentu saja, masalah keamanan dan kinerja - akan ada penolakan di sini. Kami tidak menolak hal-hal sepele: baik kami meminta untuk memperbaikinya, atau kami memperbaikinya sendiri - kami mendorong langsung ke permintaan tarik dari mereka yang membuatnya, dan kami sudah menunjukkan pada kode yang telah selesai apa, bagaimana dan mengapa kami melakukannya.
Anton Morev:Mengenai apa yang Anda katakan - apakah masalahnya sudah terpecahkan? Kebetulan seorang pengembang, saat memecahkan satu masalah, telah memecahkan masalah lain. Situasi seperti itu tidak perlu ditolak. Atau setidaknya cari tahu bagaimana tugas itu dimoderasi.
Konstantin Burkalev: Tetapi momen menghubungkan pesan commit dengan pelacak tugas tampaknya penting bagi saya. Ada tugas sehari-hari yang sangat membantu. Apakah Anda tahu kapan: "Dengar, kami melakukan hal serupa dalam masalah tentang tombol". Anda menemukan tugas tentang tombol, memahami nomornya, pergi ke log repositori, mencari perbedaan dari komit tersebut dan lihat: memang, kami telah mengacaukan ini dan itu, itu dapat dipindahkan ke sini.
Namun hal sebaliknya juga terjadi. Saya melihat kode sumber dari satu proyek dan saya menemukan fungsi action684 di backend.
Saya menulis kepada seorang teman, saya bertanya, apa nama keren ini di kode. Dia menjawab - referensi ke tugas di pelacak. “Mengapa muncul dengan nama untuk suatu fungsi, Anda menulisnya sebagai bagian dari tugas” ... Thresh, yang seharusnya tidak ada di dunia normal, tentunya.