Bagaimana cara menolak tinjauan kode yang menurut Anda tidak perlu?

89

Saya berada dalam posisi di mana saya telah diminta untuk meninjau beberapa kode yang memperbaiki masalah yang saya pikir tidak ada.

Pemecah masalah, yang lebih senior dari saya, menegaskan perbaikannya diperlukan tetapi tampaknya tidak lebih dari C + + sofistry kepada saya. Bagian dari proses penerapan kami adalah tinjauan kode, dan sebagai insinyur tertinggi ke-2 di perusahaan kecil saya diharapkan untuk meninjau perubahan.

Saya percaya bahwa pengulas sama bertanggung jawab atas perubahan kode seperti pembuat kode asli, dan saya tidak mau menerima tanggung jawab atas perubahan ini. Bagaimana cara Anda menolak ulasan ini?

James
sumber
113
Tampak jelas bagi saya. Anda menunjukkan dalam ulasan bahwa kode tersebut adalah masturbasi intelektual C ++ yang tujuannya adalah untuk memperbaiki masalah yang tidak ada. Dan kemudian, sebagai bagian dari proses peninjauan, Anda menolak kode tersebut.
user16764
86
Jangan gunakan kata-kata "masturbasi intelektual" ketika Anda menolaknya. Jika Anda menggunakan kata-kata itu Anda akan memusuhi dia, dan dia tidak akan melihat apa yang Anda katakan sebagai kritik teknis yang berpotensi benar, dia akan melihatnya sebagai serangan pribadi. Ini mungkin benar-benar masturbasi intelektual, tetapi Anda dapat benar-benar yakin bahwa dia tidak melihatnya seperti itu.
Michael Shaw
15
James - Saya mengeluarkan emosi dari pertanyaan Anda untuk memungkinkan komunitas fokus pada pertanyaan inti Anda. Seperti orang lain tunjukkan, menggunakan terminologi yang dimuat dalam ulasan Anda hanya akan memperburuk situasi yang jelas-jelas sulit.
8
C dan C ++ penuh dengan hal-hal yang tampaknya berfungsi tetapi sebenarnya adalah perilaku yang tidak terdefinisi. UB adalah cacat nyata, bahkan jika Anda tidak dapat menulis tes yang menunjukkan itu tidak berfungsi sebagaimana mestinya. Misalnya banyak kompiler menggunakan UB sebagai peluang optimisasi, dengan mengasumsikan bahwa kasus yang tidak terdefinisi tidak pernah terjadi. Sebagai contoh jika Anda digunakan size > size+1untuk memeriksa overflow pada integer yang ditandatangani, kompiler dapat dengan mudah mengganti ekspresi ini dengan false, karena overflow dari integer yang ditandatangani tidak ditentukan. Lihat blog.llvm.org/2011/05/…
CodesInChaos
5
@MichaelShaw "dia akan melihatnya sebagai serangan pribadi." yang menurut saya mirip dengan kata-kata itu, sebuah serangan pribadi terhadap seorang dev yang lebih senior yang dengannya OP memiliki perbedaan pendapat yang kini dapat "dimenangkannya" karena ia ditempatkan pada posisi berkuasa.
jwenting

Jawaban:

274

Mintalah uji kasus yang gagal tanpa perubahan yang berhasil dengan perubahan tersebut.

Jika dia tidak bisa menghasilkan satu, Anda menggunakannya sebagai pembenaran.

Jika dia dapat menghasilkan satu maka Anda perlu menjelaskan mengapa tes tidak valid.

Craig
sumber
245
Atau mungkin jika dia bisa menghasilkan satu, Anda menguji kembali asumsi Anda dan menganggap bahwa ia mungkin benar. Agaknya, inti dari latihan ini adalah untuk memperbaiki kode, bukan untuk memenangkan perdebatan.
Caleb
99
Hanya karena Anda tidak dapat menulis tes tidak berarti itu tidak rusak. Perilaku tidak terdefinisi yang biasanya bekerja sesuai yang diharapkan (C dan C ++ penuh dengan itu), kondisi balapan, potensi pemesanan ulang karena model memori yang lemah ...
CodesInChaos
29
Juga, bagaimana dengan refactoring standar? Bagaimana Anda menulis tes yang gagal untuk pekerjaan refactoring?
Mike Weller
62
"Minta dia untuk membuktikan mengapa itu perlu ..." Mungkin memintanya untuk mengajarimu mengapa itu perlu.
Mike Sherrill 'Cat Recall'
8
@RhysW: Asumsi salah. Kode yang ada, dengan kondisi balapan, tidak dapat diuji dalam hal itu juga. Jadi kode baru secara teoritis lebih baik dan setara secara empiris. Asumsi Anda hanya berlaku jika kode lama secara empiris lebih baik.
MSalters
152

Peninjau harus objektif.

Jelas bahwa Anda telah membentuk pendapat tentang kode yang dipermasalahkan bahkan sebelum Anda meninjaunya, dan sepertinya Anda dan pemecah masalah telah mempertaruhkan posisi. Jika demikian, maka Anda akan mengalami kesulitan untuk tampil objektif, dan bahkan lebih sulit lagi menjadi objektif. Tidak ada yang membantu prosesnya, dan mungkin hal terbaik dan paling obyektif yang dapat Anda lakukan adalah mengundurkan diri dengan alasan Anda terlalu dekat dengan masalah tersebut.

Pertimbangkan pendekatan tim.

Jika Anda tidak dapat menghapus diri sendiri, mungkin Anda dapat meminta beberapa insinyur lain meninjau kode secara bersamaan. Entah mereka akan setuju dengan Anda bahwa kode tersebut harus ditolak atau tidak. Jika mereka setuju dengan Anda, maka itu tidak lagi hanya Anda vs pemecah masalah, dan Anda akan dapat membuat kasus yang lebih kuat bahwa tim melihat perbaikan secara objektif dan memutuskan untuk tidak menerimanya. Di sisi lain, jika mereka memutuskan untuk menerima perbaikan maka itu juga akan menjadi keputusan tim. Itu harus berjalan tanpa mengatakan bahwa Anda harus berpartisipasi dengan pikiran yang terbuka seperti yang Anda bisa kelola, dan bahwa Anda tidak boleh mencoba untuk mempengaruhi pendapat anggota tim lain dengan apa pun selain diskusi rasional. Penting: jika ada hasil yang buruk nanti, jangan lempar tim ke bawah bus dengan mengatakan "Ya, saya selalu mengatakan itu kode yang buruk, tetapi saya kalah jumlah dengan anggota tim lainnya. "

Penolakan adalah bagian alami dari proses peninjauan kode.

Proses peninjauan kode tidak ada untuk memperbaiki stempel karet dari orang yang lebih senior; itu ada di sana untuk melindungi dan meningkatkan kualitas kode. Tidak ada yang salah dengan menolak perbaikan asalkan Anda melakukannya karena alasan yang benar, yaitu bahwa perbaikan tidak meningkatkan kode. Jika, setelah peninjauan terbuka terhadap kode, Anda masih merasa bahwa perbaikan tidak mengurangi risiko dan / atau besarnya masalah yang dapat dibuktikan, maka Anda harus menolaknya. Ini bukan pribadi, hanya pendapat jujur ​​Anda. Jika fixer tidak setuju, tidak apa-apa juga, dan pada saat itu menjadi masalah bagi manajemen untuk mencari tahu. Pastikan untuk tetap jujur, terbuka, dan profesional.

Tanggung jawab memotong kedua cara.

Anda mengatakan bahwa Anda tidak ingin bertanggung jawab atas perubahan ini, tampaknya karena Anda tidak percaya bahwa ada masalah. Namun, Anda perlu menyadari bahwa jika Anda salah dan ada adalah masalah, maka Anda mungkin berakhir menjadi bertanggung jawab untuk menolak kode yang akan menghindari masalah.

Ambil catatan.

Menyimpan catatan tertulis dari proses peninjauan akan membantu Anda menjaga fakta tetap benar. Tuliskan pikiran dan kekhawatiran Anda saat meninjau, menguraikan, dan hasil dari setiap tes yang mungkin Anda jalankan untuk mengukur dugaan masalah dan perbaikannya, dll. Jika masalah ini meningkat, Anda akan memiliki catatan tentang apa yang telah Anda lakukan untuk mendukung Anda posisi. Jika masalah muncul lagi di masa mendatang (mungkin akan terjadi jika fixer melekat pada pandangannya sendiri), Anda akan memiliki sesuatu untuk mengacaukan memori Anda.

Caleb
sumber
24
+1 ini jauh lebih berguna daripada jawaban yang diterima
Simon Bergot
2
Pastinya. Jawaban yang diterima khusus untuk satu kasus, jelas tidak umum dan dipikirkan dengan baik seperti yang satu ini.
Florian Margaine
40

Tuliskan alasan Anda untuk penolakan dan pembelaan untuk kemungkinan kontra argumen. Kemudian diskusikan secara rasional dengan pemecah masalah dan jika perlu tingkatkan ke manajemen.

Jika Anda memiliki dokumentasi yang memadai (termasuk daftar kode, hasil pengujian, dan argumen objektif ), Anda akan tercakup dengan baik.

Anda akan menyebabkan beberapa niat buruk dengan pemecah masalah, jadi pastikan masalah ini sepadan (yaitu, apakah penyebab tidak diperbaiki membahayakan?)

Juga, jika pemecah masalah terkait dengan pemiliknya, lupakan jawaban ini.

Dan Pichelman
sumber
9
Saya akan memilih dua kali untuk bagian terakhir dari jawaban Anda sendiri. Jawaban yang sangat solid.
31

Baru-baru ini di berita LinkedIn, ada sebuah artikel tentang resolusi konflik di tempat kerja dan menjadi rendah hati, bukannya terlihat arogan. Sayangnya, saya tidak dapat menemukan tautannya sekarang, tetapi intinya adalah mencoba membuat pertanyaan dengan cara yang tidak konfrontatif. Tolong jangan menganggap ini sebagai saya menyiratkan bahwa Anda sombong, itu hanya cara di mana artikel itu ditulis.

Bagaimanapun, dalam memberi tahu programmer senior bahwa dia salah dalam asumsinya hanya akan membuatnya mengambil pendekatan defensif dan menyerang Anda kembali dalam tanggapannya. Namun, jika Anda mengajukan pertanyaan mengapa ia percaya masalah itu ada, dari sudut pandang kurangnya pemahaman Anda, maka dia cenderung lebih terbuka untuk membahas masalah tersebut.

Jadi, daripada mengatakan "Masalahnya tidak ada, jadi saya tidak harus meninjau ini", mungkin bertanya sesuatu seperti "Untuk meninjau ini dengan benar, saya perlu memahami masalahnya. Bisakah Anda menjelaskannya dari Anda?" sudut pandang?"

Sebagai contoh, artikel tersebut menggambarkan tes yang diberikan kepada anak-anak di mana orang dewasa mengangkat kubus yang wajahnya semua satu warna, kecuali yang menghadap orang dewasa. Ketika anak-anak ditanya warna wajah apa yang dilihat orang dewasa, mereka yang berusia di bawah 5 tahun hampir selalu mengatakan warna yang dapat mereka lihat, karena mereka tidak dapat memahami bahwa orang dewasa dapat memiliki pandangan yang berbeda dengan wajah mereka.

Kesatria Kegelapan
sumber
8
Poin yang sangat bagus, tetapi saya gagal memahami bagaimana contoh di bagian bawah terkait (saya tidak mengklaim bahwa itu tidak berhubungan, tentu saja, hanya menunjukkan kurangnya pemahaman saya tentang hubungan).
ugoren
8
@ugoren Ha ha, saya suka apa yang Anda lakukan di sana!
TheDarkKnight
1
@ugoren hubungannya adalah 'kecuali Anda dapat melihat seluruh gambar, jangan membentuk pendapat konkret'
RhysW
2
Saya selalu menyukai pendekatan yang sederhana, saya akan menolak perubahan atas dasar yang tidak saya mengerti dan karena itu, saya tidak memenuhi syarat untuk memberikannya ok.
PhilDin
2
@ PhilDin Ada yang rendah hati dan kemudian ada yang mengatakan Anda tidak memenuhi syarat untuk pekerjaan itu. Jika dia berada dalam posisi untuk meninjau kode, saya pikir orang-orang yang menempatkannya di posisi itu berharap bahwa dia cukup berkualitas untuk melakukannya.
Andy
9

C / C ++ bisa penuh dengan perilaku yang tidak terdefinisi. Satu sisi itu buruk karena dapat menyebabkan perilaku yang tidak terduga. Di sisi lain itu memungkinkan pengoptimalan agresif dan biasanya ketika Anda menggunakan C / C ++ Anda tertarik pada kecepatan.

Menulis test case yang rusak mungkin sulit - mungkin melibatkan arsitektur aneh atau kompiler yang tidak ada lagi. Juga mungkin tampak seperti pada arsitektur yang masuk akal "itu seharusnya tidak menyebabkan masalah".

Namun pada satu titik atau lain Anda akan mengubah platform (katakan - Anda ingin mobile sehingga Anda porting ke ARM atau Anda ingin mempercepat dan menggunakan GPU). Pada titik ini hal-hal mungkin mulai rusak dan Anda perlu men-debug itu. Mungkin sepele seperti memperbarui kompiler ke versi yang lebih baru (dan Anda mungkin menginginkannya / membutuhkannya).

Kode yang bermasalah adalah:

int d[16];

int SATD (void)
{
   int satd = 0, dd, k;
   for (dd=d[k=0]; k<16; dd=d[++k]) {
     satd += (dd < 0 ? -dd : dd);
   }
   return satd;
 }

Selama iterasi terakhir d[++k] => d[++15] => d[16]diakses. Karena biasanya elemen berikutnya adalah memori legal (dalam hal paging bukan model memori C) bahkan kompiler sepele tidak menghasilkan executable dengan perilaku aneh. Satu-satunya cara menulis kasus uji adalah menemukan platform dengan model memori yang persis sama dengan C (mungkin VM).

Namun gcc 4.8 prerelase terlihat yang d[++k]dieksekusi di setiap loop. Jadi k < 16kalau tidak, akses akan ilegal dan legalitas program yang dimasukkan ke kompiler adalah bagian dari kontrak. Jadi kondisi loop selalu benar mengingat asumsi sehingga loop tak terbatas. Ini mungkin terdengar aneh tapi itu optimasi yang sangat legal - memancarkan system("dd if=/dev/zero of=/dev/sda"); system("format c:")juga akan menjadi pengganti yang tepat untuk loop. Ada cara yang lebih halus yang bisa Anda pilih untuk menunjukkan perilaku. Sebagai contoh selama kuliah tentang Memori Transaksional saya ingat bahwa presenter mencoba beberapa kali untuk mendapatkan nilai yang salah ketika 2 utas meningkatkan nilai yang sama.

Namun C / C ++, berlawanan dengan beberapa bahasa, distandarisasi sehingga perselisihan tersebut dapat dilakukan dengan mengacu pada sumber obyektif:

  • Jika Anda berpikir bahwa ini bukan masalah, coba buktikan dengan menggunakan standar C / C ++ (artinya mengarah ke nilai dan perilaku yang ditentukan)
  • Jika dia berpikir bahwa ini adalah masalah, minta dia untuk membuktikannya menggunakan standar C / C ++ (yaitu bahwa itu mengarah pada nilai atau perilaku yang tidak ditentukan)

Secara umum jika tim Anda menulis dalam C / C ++, sangat berguna untuk memiliki standar - bahkan pakar tim dapat menemukan sesuatu yang aneh sesekali.

Maciej Piechotka
sumber
4

Ini terdengar lebih seperti masalah dengan kebijakan tim Anda daripada dengan ulasan khusus ini. Ketika saya bekerja di sebuah toko perangkat lunak besar di sebuah tim yang terdiri dari 9 orang, seorang flat-out reviewer memiliki hak veto atas kode, dan ini adalah standar yang kita semua hormati. Kami cukup berbakat dan cerdas - yaitu. "dewasa", tetapi lebih dalam arti "tidak kekanak-kanakan" daripada "berpengalaman" - bahwa pimpinan tim kami dapat secara wajar mengharapkan kami untuk selalu dapat mencapai kesepakatan tanpa beralih ke argumen dalam periode waktu yang bijaksana.

Hanya berdasarkan pada bahasa Anda di pos Anda, saya mungkin memperingatkan Anda bahwa sepertinya Anda akan mendekati situasi ini dengan cara yang dapat mengarah pada argumen devolusi. Mungkin itu adalah "masturbasi intelektual," tetapi mungkin ada alasan dia melakukannya, dan beban Anda adalah menemukan cara yang lebih sederhana untuk menyelesaikan masalah itu - dan jika tidak ada cara seperti itu, catat dalam komentar mengapa kode masturbasi sebenarnya diperlukan.

Djechlin
sumber
1
"... tapi mungkin ada alasan dia melakukannya ..." - Itu asumsi besar yang kamu buat. Dan Anda juga kehilangan titik bahwa Pertanyaan menyatakan bahwa (menurut pendapat OP) tidak ada masalah. Tentunya, tanggung jawab harus berada pada orang yang mengusulkan "perbaikan" untuk menunjukkan bahwa ada masalah nyata untuk diperbaiki. Tidak ada artinya untuk mengusulkan "solusi sederhana" untuk masalah yang tidak ada.
Stephen C
4
@StephenC ya, dan saya berdiri dengan pendapat bahwa OP harus memberikan manfaat dari keraguan dalam kasus ini. Atau yang lain itu akan menjadi tinjauan yang benar-benar konfrontatif.
djechlin
3

Buat bukti acara submitter bahwa kodenya memperbaiki masalahnya. Jika dia bisa menguji, dan menunjukkan bukti kepada Anda, maka Andalah yang salah dan harus berhenti dari keluhan Anda dan mengatasinya. Jika dia tidak dapat menunjukkan bukti untuk mendukung klaimnya ada masalah, dan menunjukkan bukti bahwa tambalannya memperbaiki masalah, maka saya akan mengirimnya kembali ke papan gambar.

Tentu saja mungkin ada kepekaan politis internal di sekitar ini. Tapi ini adalah cara saya pergi (enak).

SnakeDoc
sumber