Kod İnceleme En İyi Uygulamaları

İnternet, kod incelemeleri konusunda çok çeşitli materyaller sağlar: kod incelemelerinin şirket kültürü üzerindeki etkisi, resmi güvenlik incelemeleri, daha kısa rehberler, daha uzun kontrol listeleri, insanlaştırılmış yorumlar, ilk olarak kod incelemeleri yapmanın nedenleri, en iyi uygulamalar, daha fazlası uygulamalar, hata bulma için kod inceleme etkinliği istatistikleri ve kod inceleme örnekleri yanlış gitti. Oh, tabii ki de kitaplar var. Uzun lafın kısası, bu blog yazısı Palantir’in kod incelemelerini almasını sağlıyor. Hakem değerlendirmesinde derin kültürel isteksizliğe sahip kuruluşlar, bu kılavuzu izlemeye çalışmadan önce Karl E. Wiegers’ın Humanizing Akran Değerlendirmeleri hakkındaki mükemmel makalesine bakmak isteyebilir.

Bu gönderi, Java Kod Kalitesi araç zinciri, Baseline'ın en iyi uygulama kılavuzlarından kopyalanmıştır ve aşağıdaki konuları kapsar:

  • Neden, ne ve ne zaman kod incelemeleri yapılmalı
  • Kodu incelemeye hazırlama
  • Kod incelemeleri gerçekleştirme
  • Kod inceleme örnekleri

Motivasyon

Kod kalitesini artırmak ve ekip ve şirket kültürü üzerindeki olumlu etkilerden yararlanmak için kod incelemeleri (CR'ler) yapıyoruz. Örneğin:

  • Taahhütçiler, değişim talebini gözden geçirecek bir takım hakemler anlayışıyla motive olurlar: işletmeci, gevşek uçları temizleme, TODO'ları birleştirme ve genel olarak taahhüdü geliştirme eğilimindedir. Kodlama uzmanlığının meslektaşlar aracılığıyla tanınması birçok programcı için bir gurur kaynağıdır.
  • Bilgi paylaşımı, geliştirme ekiplerine çeşitli şekillerde yardımcı olur:
    - Bir ÇS, daha sonra yapılan işi temel alan takım üyelerine eklenen / değiştirilen / kaldırılan işlevselliği açıkça iletir.
    - Üretici, hakemlerin öğrenebileceği bir teknik veya algoritma kullanabilir. Daha genel olarak, kod incelemeleri kuruluş genelinde kalite çubuğunu yükseltmeye yardımcı olur.
    - Gözden geçirenler, programlama teknikleri veya değişimin iyileştirilmesine veya birleştirilmesine yardımcı olabilecek kod tabanı hakkında bilgi sahibi olabilir; örneğin, başka biri aynı anda benzer bir özellik veya düzeltme üzerinde çalışıyor olabilir.
    - Olumlu etkileşim ve iletişim, ekip üyeleri arasındaki sosyal bağları güçlendirir.
  • Bir kod tabanında tutarlılık, kodun okunmasını ve anlaşılmasını kolaylaştırır, hataları önlemeye yardımcı olur ve normal ve göçmen geliştirici türler arasında işbirliğini kolaylaştırır.
  • Kod parçalarının okunaklı olması, beyin çocuğu olan yazar için yargılanmak zordur ve tam bağlamı olmayan bir eleştirmen için yargılamak kolaydır. Okunaklı kod daha yeniden kullanılabilir, hatasız ve geleceğe yöneliktir.
  • Kaza sonucu hatalar (örneğin yazım hataları) ve yapısal hatalar (örneğin, ölü kod, mantık veya algoritma hataları, performans veya mimari kaygıları), eleştirel eleştirmenlerin dış bakış açısıyla tespit edilmesi genellikle daha kolaydır. Araştırmalar, kısa ve gayri resmi kod incelemelerinin bile kod kalitesi ve hata sıklığı üzerinde önemli bir etkiye sahip olduğunu bulmuştur.
  • Uyum ve düzenleyici ortamlar sıklıkla gözden geçirme talep eder. CR'ler genel güvenlik tuzaklarından kaçınmanın harika bir yoludur. Özelliğiniz veya ortamınız önemli güvenlik gereksinimlerine sahipse, yerel güvenlik önlemlerinin gözden geçirmesinden faydalanacak (ve muhtemelen de gerektirecektir) (OWASP’in kılavuzu bu sürecin iyi bir örneğidir).

Ne gözden geçirilir?

Bu soruya ebedi olarak doğru bir cevap yoktur ve her geliştirme ekibi kendi yaklaşımı konusunda hemfikir olmalıdır. Bazı ekipler ana şubeyle birleştirilen her değişikliği incelemeyi tercih ederken, diğerleri gözden geçirmenin gerekmediği bir "önemsizlik" eşiğine sahip olacaklar. Değişim, mühendislerin (hem yazarların hem de hakemlerin) zamanının etkin kullanımı ve kod kalitesinin korunması arasındadır. Bazı düzenleyici ortamlarda, önemsiz değişiklikler için bile kod incelemesi gerekebilir.

Kod incelemeleri sınıfsızdır: takımdaki en kıdemli kişi olmak, kodunuzun incelenmesine gerek olmadığı anlamına gelmez. Nadir bir durumda kod kusursuz olsa bile, inceleme mentorluk ve işbirliği için bir fırsat sağlar ve asgari olarak kod tabanındaki kod anlayışını çeşitlendirir.

Ne zaman gözden geçirilir?

Kod incelemeleri, otomatik kontroller (testler, stil, diğer CI) başarıyla tamamlandıktan sonra yapılmalıdır, ancak kod, depo ana hattına bağlanmadan önce. Genel olarak, son sürümden bu yana toplu değişikliklerin resmi kod incelemesini yapmıyoruz.

Ana hat dalına tek bir ünite olarak birleştirilmesi gereken ancak makul bir CR'ye sığmayacak kadar büyük olan karmaşık değişiklikler için, yığınlanmış bir CR modelini göz önünde bulundurun: Bir birincil şube özelliği / büyük özellik ve çok sayıda ikincil dal oluşturma (örneğin, özellik / big-özellik-api, özellik / büyük özellik testi, vb.) her biri işlevselliğin bir alt kümesini kapsıyor ve özellik / büyük özellik dalına göre ayrı ayrı kod incelemesi alıyor. Tüm ikincil dallar özellik / büyük özellik olarak birleştirildiğinde, ikincisini ana dalla birleştirmek için bir CR oluşturun.

Kodu incelemeye hazırlama

Hakemlerin zamanını ve motivasyonunu boşa harcamamak için incelemesi kolay CR'leri sunmak yazarın sorumluluğundadır:

  • Kapsam ve boyut Değişiklikler, kapsamlı bir şekilde kapsadığı dar, iyi tanımlanmış, kendi kendine yeten bir kapsama sahip olmalıdır. Örneğin, bir değişiklik yeni bir özellik uygulayabilir veya bir hatayı düzeltebilir. Daha kısa değişiklikler daha uzun olanlara göre tercih edilir. Bir CR ~ 5'ten fazla dosyada önemli değişiklikler yaparsa veya yazması 1-2 günden fazla sürerse veya incelemesi 20 dakikadan uzun sürerse, onu bağımsız çoklu CR'lere bölmeyi düşünün. Örneğin, bir geliştirici, arabirimleri ve dokümantasyon açısından API'yi yeni bir özellik için tanımlayan bir değişiklik ve bu arabirimler için uygulamalar ekleyen ikinci bir değişiklik gönderebilir.
  • Yalnızca eksiksiz, kendi kendini gözden geçiren (farkına göre) ve kendi kendini test eden CR'ler gönderin. Gözden geçirenlerin zamanından tasarruf etmek için, gönderilen değişiklikleri test edin (yani, test takımını çalıştırın) ve hakemleri atamadan önce hem yerel hem de CI sunucularında tüm testlerin yanı sıra tüm testlerin ve kod kalite kontrollerinin geçtiğinden emin olun.
  • Yeniden yapılan değişiklikleri değiştirmek davranışları değiştirmemelidir; tersine, davranış değişikliği yapılan değişiklikler yeniden düzenlemekten ve kod biçimlendirme değişikliklerinden kaçınmalıdır. Bunun çok iyi nedenleri var:
    - Değişikliklerde değişiklik yapmak çoğu zaman birçok satır ve dosyaya dokunur ve sonuç olarak daha az dikkatle incelenir. İstenmeyen davranış değişiklikleri kimse fark etmeden kod tabanına sızabilir.
    - Büyük yeniden yapılanma değişiklikleri kiraz toplama, yeniden yapılanma ve diğer kaynak kontrol sihrini bozar. Havuz genelinde yeniden yapılanma taahhüdünün bir parçası olarak sunulan bir davranış değişikliğini geri almak çok zordur.
    - Pahalı insan gözden geçirme süresi, stil, sözdizimi veya biçimlendirme tartışmaları yerine program mantığına harcanmalıdır. Bunları Checkstyle, TSLint, Baseline, Prettier, vb. Gibi otomatik takımlarla yerleştirmeyi tercih ediyoruz.

Mesajları gönder

Aşağıdaki, yaygın olarak alıntı yapılan bir standardın ardından iyi bir taahhüt mesajı örneğidir:

Büyük harf, kısa (80 karakter veya daha az) özet
Gerekirse daha ayrıntılı açıklayıcı metin. Yaklaşık 120 karaktere kadar kaydırın. Bazı bağlamlarda, ilk
satır bir e-postanın konusu ve metnin geri kalanını gövde olarak değerlendirilir. Ayıran boş satır
Vücudun özeti kritiktir (bedeni tamamen ihmal etmezseniz); Eğer çalışırsanız rebase gibi araçlar karışık olabilir
ikisi birlikte.
Taahhüt mesajınızı zorunlu: "Düzeltilen hata" ve "Düzeltilen hata" ya da "Düzeltilen hata" değil. Bu sözleşme eşleşiyor
git birleştirme ve git gibi geri komutları tarafından oluşturulan taahhüt mesajları ile.
Diğer paragraflar boş satırlardan sonra gelir.
- Mermi noktaları da iyi

Hem taahhüdün ne değiştiğini hem de nasıl yaptığını tanımlamaya çalışın:

> KÖTÜ. Bunu yapma
Tekrar derle
> Güzel.
IntelliJ derlemesini düzeltmek için jcsv bağımlılığı ekleyin

Hakemleri bulmak

Üreticinin, kod tabanına aşina olan bir veya iki hakem teklif etmesi gelenekseldir. Genellikle, hakemlerden biri proje lideri veya kıdemli bir mühendisdir. Proje sahipleri, yeni CR'lerden haberdar olmak için projelerine abone olmayı düşünmelidir. Üçten fazla taraf arasındaki kod incelemeleri genellikle verimsizdir veya hatta üretkendir, çünkü farklı gözden geçirenler çelişkili değişiklikler önerebilir. Bu, doğru uygulama konusundaki temel uyuşmazlığı gösterebilir ve örneğin daha yüksek bant genişlikli bir forumda, örneğin şahsen veya ilgili tüm taraflarla yapılan bir video konferansında kod incelemesi dışında çözülmelidir.

Kod incelemeleri gerçekleştirme

Bir kod incelemesi, farklı ekip üyeleri arasında bir senkronizasyon noktasıdır ve bu nedenle ilerlemeyi engelleme potansiyeline sahiptir. Sonuç olarak, kod incelemelerinin hızlı olması (günlerin değil, saatlerin sırasına göre) olması ve ekip üyelerinin ve liderlerin zaman taahhüdünün farkında olmaları ve buna göre gözden geçirme zamanlarına öncelik vermeleri gerekir. Bir incelemeyi zamanında tamamlayabileceğinizi düşünmüyorsanız, lütfen derhal kuruluşa bildirin, böylece başka birini bulabilirler.

Gözden geçirme, gözden geçiricinin değişikliği başka bir geliştiriciye makul bir ayrıntı düzeyinde açıklayabilmesi için yeterince ayrıntılı olmalıdır. Bu, kod tabanının ayrıntılarının tek bir kişi tarafından bilinmesini sağlar.

Bir gözden geçiren olarak, kodlama standartlarını uygulamak ve kaliteyi yüksek tutmak sizin sorumluluğunuzdadır. Kodları incelemek bir bilimden çok bir sanattır. Bunu öğrenmenin tek yolu yapmak; deneyimli bir gözden geçiren, daha az deneyimli diğer gözden geçiricileri değişikliklerine koymayı düşünmeli ve önce bir inceleme yapmalarını sağlamalıdır. Yazarın yukarıdaki yönergeleri takip ettiğini varsayarak (özellikle kendi kendini inceleme ve kodun çalışmasını sağlama konusunda), bir gözden geçirenin kod incelemesinde dikkat etmesi gereken şeylerin bir listesi:

amaç

  • Bu kod yazarın amacını yerine getiriyor mu? Her değişikliğin kendine özgü bir nedeni olmalı (yeni özellik, refactor, bugfix, vb.). Gönderilen kod aslında bu amacı yerine getiriyor mu?
  • Sorular sor. İşlevler ve sınıflar bir sebepten dolayı mevcut olmalıdır. Sebep, inceleyen için net olmadığında, kodun yeniden yazılması veya yorum veya testlerle desteklenmesi gerektiğinin bir göstergesi olabilir.

uygulama

  • Sorunu nasıl çözeceğinizi düşünün. Farklıysa neden? Kodunuz daha fazla (uç) durumla mı ilgileniyor? İşlevsel olarak daha mı kısa / kolay / temizleyici / daha hızlı / daha güvenli mi? Geçerli kod tarafından yakalanmayan, tespit ettiğiniz bazı altlıklar var mı?
  • Yararlı soyutlamalar için potansiyel görüyor musunuz? Kısmen çoğaltılmış kod genellikle daha soyut veya genel bir işlevsellik parçasının çıkarılabileceğini ve daha sonra farklı bağlamlarda yeniden kullanılabileceğini gösterir.
  • Bir rakip gibi düşün, ama bu konuda iyi davran. Kodlarını kıran problemli konfigürasyonlar / giriş verilerini bularak kısayolları veya eksik vakaları alan yazarları “yakalamaya” çalışın.
  • Kütüphaneleri veya mevcut ürün kodunu düşünün. Birisi mevcut işlevselliği yeniden uyguladığında, bunun zaten mevcut olduğunu bilmediklerinden daha sık değil. Bazen, kod ya da işlevsellik, örneğin bağımlılıkları önlemek amacıyla, bilerek kopyalanır. Bu gibi durumlarda, bir kod yorumu amacını netleştirebilir. Tanıtılan işlevler zaten mevcut bir kütüphane tarafından sağlandı mı?
  • Değişim standart kalıpları takip ediyor mu? Oluşturulan kod tabanları genellikle adlandırma kuralları, program mantığı ayrıştırması, veri türü tanımları, vb. Etrafında kalıplar gösterir. Değişikliklerin mevcut kalıplara göre uygulanması genellikle arzu edilir.
  • Değişim, derleme zamanı veya çalışma zamanı bağımlılıkları ekler mi (özellikle alt projeler arasında)? Ürünlerimizi mümkün olduğunca az bağımlılıkla gevşek bir şekilde bir arada tutmak istiyoruz. Bağımlılık ve yapı sistemi üzerinde yapılan değişiklikler yoğun biçimde incelenmelidir.

Okunabilirlik ve stil

  • Okuma deneyiminizi düşünün. Kavramları makul bir sürede kavradınız mı? Akış aklı başında ve değişken ve yöntem adlarını takip etmek kolay mıydı? Birden fazla dosya veya işlevi takip edebildiniz mi? Tutarsız adlandırma ile ertelendi misiniz?
  • Kod, kodlama kurallarına ve kod stiline uygun mu? Kod, stil, API sözleşmeleri vb. Açısından projeyle tutarlı mı? Yukarıda belirtildiği gibi, stil tartışmalarını otomatik araçlarla düzenlemeyi tercih ediyoruz.
  • Bu kod TODO'lara sahip mi? TODO'lar kodda yığıldı ve zamanla bayatladılar. Yazara GitHub Issues veya JIRA ile ilgili bir bilet gönderin ve konu numarasını TODO'ya ekleyin. Önerilen kod değişikliği, yorumlanmış kod içermemelidir.

İdame

  • Testleri oku. Testler yoksa ve yapılması gerekiyorsa, yazardan bazılarını yazmasını isteyin. Gerçekten denenmeyen özellikler nadirdir, ancak özelliklerin denenmemiş uygulamaları maalesef yaygındır. Testleri kendileri kontrol edin: İlginç vakaları kapsıyorlar mı? Okunabilir mi? CR genel test kapsamını azaltır mı? Bu kodun bozabileceği yolları düşünün. Testler için stil standartları çoğu zaman temel koddan farklıdır, ancak yine de önemlidir.
  • Bu CR test kodunu kırma, istifleme veya entegrasyon testlerini bozma riskini getiriyor mu? Bunlar genellikle ön işleme / birleştirme kontrollerinin bir parçası olarak kontrol edilmez, ancak aşağı inmeleri herkes için acı vericidir. Aranacak belirli şeyler şunlardır: test araçlarının veya modlarının kaldırılması, konfigürasyondaki değişiklikler ve yapay mizanpaj / yapıdaki değişiklikler.
  • Bu değişiklik geriye dönük uyumluluğu bozuyor mu? Eğer öyleyse, değişikliği bu noktada birleştirmek uygun mudur yoksa daha sonraki bir sürümde kullanılmalı mı? Sonları, veritabanı veya şema değişiklikleri, genel API değişiklikleri, kullanıcı iş akışı değişiklikleri vb. İçerebilir.
  • Bu kod entegrasyon testlerine ihtiyaç duyuyor mu? Bazen, kod yalnızca dış ünite sistemleriyle veya yapılandırmasıyla etkileşime giriyorsa, yalnızca birim testleriyle yeterince test edilemez.
  • Kod düzeyinde belgeler, yorumlar ve taahhüt mesajları hakkında geri bildirim bırakın. Gereksiz yorumlar kodu bozar ve kısa ve öz taahhüt mesajları gelecekteki katılımcıları gizemli kılar. Bu her zaman geçerli değildir, ancak kaliteli yorumlar ve taahhüt mesajları satır boyunca kendileri için ödeme yapar. (Mükemmel ya da gerçekten korkunç bir mesaj ya da yorum yaptığınız zamanı düşünün.)
  • Dış dokümanlar güncellendi mi? Projeniz bir README, CHANGELOG veya başka bir belgeye sahipse, değişiklikleri yansıtacak şekilde güncellendi mi? Eski belgeler hiç olmadığı kadar kafa karıştırıcı olabilir ve gelecekte düzeltmek, şimdi güncellemekten daha maliyetli olacaktır.

Kısa / okunabilir / verimli / şık kodları övmeyi unutmayın. Tersine, bir CR'yi reddetmek veya reddetmek kaba değildir. Değişiklik fazlalık veya alakasız ise, bir açıklama ile reddedin. Bir veya daha fazla ölümcül kusur nedeniyle kabul edilemez olduğunu düşünüyorsanız, bir açıklama ile tekrar onaylamayın. Bazen bir CR'nin doğru sonucu “bunu tamamen farklı bir şekilde yapalım” ya da “bunu hiç yapmayalım” dır.

Hakemlere saygılı olun. Karşılıklı düşünme kullanışlı olsa da, bu sizin özelliğiniz değildir ve tüm kararları veremezsiniz. İncelenenle kodunuzla olduğu gibi bir anlaşmaya varamazsanız, gerçek zamanlı iletişime geçin veya üçüncü bir görüş isteyin.

Güvenlik

API uç noktalarının, kod tabanının geri kalanıyla tutarlı olarak uygun yetkilendirme ve kimlik doğrulama gerçekleştirdiğini doğrulayın. Diğer zayıf yönleri, örneğin zayıf yapılandırma, kötü niyetli kullanıcı girişi, eksik günlük olayları, vb. Olup olmadığını kontrol edin. Şüphe duyuyorsanız, CR'yi bir uygulama güvenliği uzmanına yönlendirin.

Yorumlar: özlü, dost canlısı

Yorumlar özlü ve tarafsız bir dilde yazılmalıdır. Kodu eleştirin, yazarı değil. Bir şey net değilse, cehalet almak yerine açıklığa kavuşturun. Özellikle değerlendirmelerle birlikte iyelik zamirlerinden kaçının: “kodum değişiklikten önce işe yaradı”, “yönteminiz bir hata”, vb. Kesin kararlardan kaçının: “bu asla işe yaramaz”, “sonuç her zaman yanlıştır”.

Öneriler (örneğin, “Öneri: okunabilirliği arttırmak için özüt yöntemi”), gerekli değişiklikler (örneğin, “Eklenti Ekle”) ve tartışma veya açıklama gerektiren noktalar (örneğin, “Bu gerçekten doğru davranış mı?) Arasında ayrım yapmayı deneyin. öyleyse, lütfen mantığı açıklayan bir yorum ekleyin. ”). Bir sorunun ayrıntılı açıklamalarına bağlantılar veya işaretçiler sağlamayı düşünün.

Bir kod incelemesi bittiğinde, yazarın yorumlarınıza ne ölçüde yanıt vermesini beklediğinizi ve değişikliklerin uygulanmasından sonra CR'yi tekrar gözden geçirmek isteyip istemediğinizi belirtin (örneğin, “Yanıt verdikten sonra birleşmekten çekinmeyin birkaç öneriye ”ve“ Lütfen önerilerimi dikkate al ve başka bir bakış açabileceğimde bana haber ver. ”).

Yorumlara cevap verme

Kod incelemesinin amacının bir kısmı, yazarın değişiklik isteğini iyileştirmektir; Sonuç olarak, incelemenizin önerileriyle rahatsız edilmeyin ve kabul etmeseniz bile bunları ciddiye alın. Her yoruma cevap verin, sadece basit bir “ACK” veya “bitti” bile olsa. Neden belirli kararlar verdiğinizi, neden bazı fonksiyonların mevcut olduğunu açıklayın. zaman iletişimi veya dış görüş isteyin.

Düzeltmeler aynı şubeye itilmelidir ancak ayrı bir taahhütte bulunulmalıdır. Gözden geçirme sürecinde Squashing, gözden geçirenin değişiklikleri takip etmesini zorlaştırıyor.

Farklı ekiplerin farklı birleştirme politikaları vardır: bazı ekipler yalnızca proje sahiplerinin birleşmesine izin verirken, diğer ekipler olumlu bir kod incelemesinden sonra katılımcının birleşmesine izin verir.

Kişisel kod değerlendirme

Kod incelemelerinin çoğunluğu için Reviewable, Gerrit veya GitHub gibi asenkron diff tabanlı araçlar mükemmel bir seçimdir. Çok farklı uzmanlığa veya deneyime sahip taraflar arasındaki karmaşık değişiklikler veya incelemeler, aynı ekran veya projektörün önünde veya VTC veya ekran paylaşımı araçlarıyla uzaktan kişisel olarak gerçekleştirildiğinde daha verimli olabilir.

Örnekler

Aşağıdaki örneklerde önerilen inceleme yorumları, kod bloklarındaki // R: ... açıklamaları ile belirtilmiştir.

Tutarsız adlandırma

sınıf MyClass {
  özel int countTotalPageVisits; // R: sürekli değişkenleri adlandır
  özel int uniqueUsersCount;
}

Tutarsız yöntem imzaları

arayüz MyInterface {
  / ** s ayıklanamıyorsa {@link İsteğe bağlı # boş} döndürür. * /
  public İsteğe bağlı  extractString (String s);
  / ** {@code s} yeniden yazılamazsa null değerini döndürür. * /
  // R: dönüş değerlerini uyumlaştırmalı: burada da İsteğe bağlı <> kullanın
  public Dize rewriteString (Dize s);
}

Kütüphane kullanımı

// R: kaldır ve Guava'nın MapJoiner'ı ile değiştir
String joinAndConcatenate (Harita  harita, Dize keyValueSeparator, Dize keySeparator);

Kişisel zevk

int dayCount; // R: nit: Ben genellikle numFoo'yu fooCount'a tercih ederim; size kalmış, ancak bu projede tutarlı kalmalıyız.

Hatalar

// R: Bu işlem rakam + 1 yineleme gerçekleştiriyor, bu kasıtlı mı?
// Eğer öyleyse, sayısal anlambilgisini değiştirmeyi düşünün.
(int i = 0; i <= numIterations; ++ i) {
  ...
}

Mimari kaygılar

otherService.call (); // R: Bence OtherService'e bağımlılıktan kaçınmalıyız. Bunu şahsen tartışabilir miyiz?

Yazarlar

Robert F (GitHub / Twitter)