Kod İnceleme: Kapı Bekçiliği Değil, Bilgi Yayma

Sahadan: 41 yorum, 0 mimari

O PR 780 satırdı ve üç günlük bir işi tek parçada getiriyordu. İncelemeyi yapan kişi kötü bir mühendis değildi — tam tersi, ekibin en dikkatlisiydi. Ama 780 satır açtığında insan beyni okumayı bırakıp taramaya geçiyor. Tarayınca da gözüne ne çarpıyorsa onu yazıyorsun: boşluklar, değişken adları, eksik bir log satırı.

Mimari itirazlar dikkat değil, bağlam istiyor. “Bu servis şunu doğrudan çağırmamalı” demek için iki servisin ilişkisini kafada tutman gerekiyor, ve 780 satırın 400’üncü satırında o alan çoktan dolmuş oluyor.

Benim hatam şuydu: PR’ı açan kişiye “bunu böl” demek yerine, incelemeye “daha dikkatli bak” dedim. Yanlış tarafı zorluyordum. Üç ay sonraki kesintiden sonra kuralı değiştirdik: bölmek yazarın işi, dikkat incelemecinin işi değil.

PR boyutu ile değerli yorum arasındaki ters oran

Üç aylık PR kayıtlarımızı boyutlarına göre ayırdığımızda tablo şuydu:

Son satır en ilginç olanı: 800 satırı geçen PR’lar az yorum alıyor. Çünkü o noktada incelemeci pes ediyor ve “uygun” yazıyor. Yani büyük PR daha çok itiraz üretmiyor, itirazı tamamen susturuyor.

Bölme işi de göründüğü kadar zor değil, ama yazarın kafasında planlanması gerekiyor: önce şema değişikliği (ayrı sürüm), sonra yeni kod kapalı flag arkasında, sonra flag’i açan küçük PR. Üç ayrı inceleme, üçü de okunabilir.

Yorum etiketleri: en ucuz iyileştirme

İnceleme kültüründe tek hamlede en çok işe yarayan şey bu oldu. Her yorum bir etiketle başlıyor:

engelleyici: Bu satir odeme tutarini float tutuyor. Kurusu
             kaybediyoruz; decimal olmali. Birlesmeden once degismeli.

oneri:       Bu dongu yerine tek sorguda yapilabilir. Simdi
             sart degil, ama sonra ariza cikarirsa buradan bak.

nit:         Degisken adi "x" yerine "kalan_tutar" olabilir.
             Ilgilenme, sadece soyluyorum.

soru:        Burada neden retry yok? Bilmiyorum, ogrenmek istiyorum.
             (Bu etiket incelemeciyi de ogrenen tarafa koyuyor.)

İki etiket özellikle güçlü. nit, incelemecinin fikrini söylemesini sağlıyor ama yazarı serbest bırakıyor — bu olmadan her küçük tercih bir pazarlığa dönüşüyor. soru ise incelemeyi tek yönlü denetimden çıkarıyor; kıdemsiz birinin kıdemliye soru sorması, o PR’da öğrenen tek kişinin yazar olmadığını gösteriyor.

engelleyici etiketinin tek kuralı var: neden engellediğini ve neyin değişmesi gerektiğini tek cümleyle yaz. “Bu yaklaşım bana yanlış geldi” engelleyici bir yorum değil, bir histir; bir toplantıya taşınır, PR’da durmaz.

Bekleme süresi: sessiz kalite katili

PR’ın ne kadar beklediğini ölçmeye başlayana kadar bunun bir kalite meselesi olduğunu fark etmemiştim. Bekleyen PR’da üç şey aynı anda bozuluyor:

• Yazar bağlamı kaybediyor. İki gün sonra gelen yoruma cevap vermek, yazmaktan uzun sürüyor.
• Dal çatışmaya başlıyor. Bekleyen dal, birleşirken en çok çatışan daldır; çatışma çözümü de incelenmemiş kod üretiyor.
• Yazar PR’ı büyütüyor. Beklerken sıkılan insan üstüne bir şey daha ekliyor ve PR yukarıdaki tablonun son satırına doğru kayıyor.

Koyduğumuz kural tek cümle: inceleme, kendi sprint işinden önce gelir. Sabah ilk iş açık PR’lara bakılır. Kulağa verimsiz geliyor — değil: senin 30 dakikan, başkasının iki gününü serbest bırakıyor.

Kim onaylar, kaç kişi onaylar

Onaylayacak kişinin kim olduğu da sahiplikle belirlenmeli, gönüllülükle değil. Dosya sahipliği tanımlı değilse inceleme her zaman aynı iki kişiye düşüyor ve o iki kişi ekibin darboğazı oluyor — monorepo yazısında anlattığım sınır meselesinin aynısı.

Yazarın sorumluluğu

İnceleme kalitesinin yarısı, PR açılmadan önce belirleniyor. Yazardan beklediğimiz üç şey var:

1. Kendi PR’ını önce kendin incele. Açmadan önce değişiklikleri baştan sona oku. Bulduğun şeyleri kimse okumak zorunda kalmıyor; bu tek alışkanlık yorum sayısını görünür şekilde düşürüyor.
2. Açıklamada “neden”i yaz. Ne yaptığını kod zaten söylüyor. Neden bu yolu seçtiğini, hangi alternatifi elediğini ve neyi bilerek yapmadığını yaz.
3. Gözden geçirenin nereye bakmasını istediğini söyle. “Asıl riskli yer şu fonksiyon” demek, incelemeyi dağıtmaktan kurtarıyor.

Ne izlemeli?

Bende işe yaramayanlar

• İnceleme kontrol listesi asmak. On maddelik bir liste yaptık; iki hafta sonra kimse bakmıyordu. İşe yarayan şey listeyi otomatikleştirmek oldu: biçim, lint ve basit kalıplar CI’a taşındı. İnsanın bakacağı şey sadece insanın bakabileceği şey olmalı.
• “Herkes herkesi incelesin” rotasyonu. Adil görünüyordu; pratikte hiç bilmediği alana bakan kişi sadece biçim yorumu yazabiliyordu. Sahiplik + kasıtlı eşleştirme daha iyi çalıştı: yeni alan öğrenecek kişi, o alanın PR’ına ikinci incelemeci olarak eklendi.
• Onay sayısını ikiye çıkarmak. Bir kesintiden sonra refleksle yaptık. Hata oranı düşmedi; bekleme süresi iki katına çıktı. Sonra sadece yukarıdaki listedeki alanlara indirdik.

Kontrol listesi

Sonuç

O 780 satırlık PR’da kimse hata yapmadı. Yazar işini bitirdi, incelemeci dikkatle baktı, 41 yorum yazdı. Süreç çalıştı ve yine de üç ay sonra bir kesinti çıktı. Çünkü incelemeden beklediğimiz şeyle ona verdiğimiz koşullar uyuşmuyordu: mimari itiraz istiyorduk, elimize tarama gerektiren bir yığın veriyorduk.

İnceleme, kodun geçip geçmeyeceğine karar veren bir kapı olarak kurgulanırsa en çok biçim yakalar. Bilginin ekipte yayıldığı bir kanal olarak kurgulanırsa mimariyi yakalar — ve biçimi zaten CI yakalar.

Tek soruyla ölç: son ayki incelemelerde kaç kez “bunu bilmiyordum, öğrendim” dendi? Cevap sıfırsa incelemen çalışıyor olabilir, ama işe yaramıyor.