Code Quality Keepersopowieści z pola bitwy o jakość kodu

Rafał Głowiński, Bartosz WalacikConfitura 2015

O nas

Rafał Głowiński @GlowinskiRafal

Bartosz Walacik @BartoszWalacik

http://javers.org

O czym będziemy mówić

Pytania:

￫ Jak dbać o jakość kodu w zespole? Code review?

￫ A co jeśli mamy 40+ zespołów?

￫ Jak zadbać o jakość na poziomie całej organizacji?

O czym będziemy mówić

Opowiemy:

￫ Po co CQK?

￫ Jak zdefiniować kryteria jakości i ocenić kod różnych zespołów

￫ Jak robimy prawdopodobnie największe Code Review w Polsce

￫ Co udało nam się znaleźć: “perełki”, typowe błędy

￫ A także… co udało nam się osiągnąć

￫ 40+ zespołów (dev + infra)

Zespoły w Allegro

￫ 4 miasta (Poznań, Toruń, Warszawa, Kraków)

￫ Często doświadczenie głównie w PHP

￫ Większość pisze i utrzymuje mikrousługi na JVM

Jak powstało CQK i po co?

Pierwszy skład zespołu CQK:

￫ doświadczeni inżynierowie

￫ zaangażowani w community

￫ autorytet nie hierarchia

￫ kilka osób myślących podobnie, startup

Jak powstało CQK i po co?

Pierwszy Cel:Przejrzeć i ocenić (w miarę) obiektywnie cały kod

Pierwsze review:￫ 1 dzień w 1 salce

￫ kilkadziesiąt projektów do przejrzenia

￫ 15 minut na projekt (!)

￫ totalna partyzantka

Jak powstało CQK i po co?

Wnioski z pierwszego review:￫ 15 min na projekt to za mało

￫ potrzebna jest metodyka i organizacja

￫ koncentracja na wybranych aspektach

Nowe Cele:￫ Cel 1: podnoszenie jakości kodu w całej organizacji

poprzez review, feedback i mentoring

￫ Cel 2: wkład do ocen technicznych zespołów

Jak pracujemy

￫ Zaczęliśmy od garstki osób, teraz jest nas więcej

￫ Co kwartał zmieniamy metodykę review

￫ Merytoryczny feedback

￫ Przejście na tryb “push”

Metodyka review: pierwszy kwartał

“Wrażenia artystyczne”(15 min na projekt…)

Metodyka review: drugi kwartał

Duże i częste naruszenia podstawowych zasad

Są błędy, ale generalnie jest ok

Jest dobrze!

Magic Servlet Antipattern is Back:￫ logika biznesowa/dostęp do repozytorium w kontrolerze

￫ nieprawidłowe wykorzystanie frameworków (np. ręczne

mapowanie wyjątków na kodu HTTP)

Metodyka review: drugi kwartał

Zwięzłość i czytelność testów:￫ struktura testów, prawidłowe wykorzystanie bibliotek

￫ co testują testy

￫ mocks (overmocking, mockowanie nie swoich klas)

Szkolenia: trzeci kwartał

￫ Naming i omówienie bytów w testach (Stub, Mock, Spy)

￫ Spock: Data Driven, wyjątki, asercje, Groovy

￫ (*) JUnit: Mockito, AssertJ, JUnitParams, catch-exception

￫ Overmocking + verify = betonowy kod

￫ Testy integracyjne: bazy danych, REST

Zwięzłość i czytelność testów cd.

Metodyka review: aktualnie

Model domenowy:

￫ klasy modelu

￫ organizacja pakietów

￫ powiązanie z bibliotekami / infrastrukturą

Metodyka review: aktualnie

Model domenowy:

brak modelu domenowego: mutowalne & anemiczne POJO

operujące na nich klasy Service

Metodyka review: aktualnie

Model domenowy:

kod jest podzielony horyzontalnie

domena powiązana z infrastrukturą (zapytania do baz danych, skomplikowane logowanie/monitoring, itp.)

widać starania odejścia od anemiczności modelu (rozsądne podejście do mutowalności, obiekty zyskują funkcjonalności)

Metodyka review: aktualnie

Model domenowy:

ładny model domenowy zgodny z OOP

struktura pakietów zorientowana domenowo

domena jest wyraźnie odseparowana od infrastruktury

podział pakietowy zgodnie z wytycznymi np. Hexagonal

Architecture

Eksperyment: review kodu JS

Opowiemy o tym za rok ;)

Główna działalność CQK

Tworzenie feedbacków dla zespołów

￫ Zespół zgłasza do feedbacku projekt lub PR

￫ Przeciętny projekt ma ~10KLOC

Co to jest feedback

Kilkustronicowy dokument na WIKI, lista punktów, np:dobre testy integracyjne (np BlacklistEndpointIntegrationSpec.groovy)

SalesRegulationsFilterTest.java - do testu tego rodzaju prostych funkcji używajciepodejścia data-driven test

Klasa Pdf.java mogłyby być immutable, czyli zamiast:Pdf pdf = new Pdf();

pdf .setContent(documentContent);

lepiejPdf pdf = new Pdf (documentContent);

długa implementacja equals() w BlacklistRequest.java. Łatwo się pomylić w takim kodzie. Czy ten equals() jest do czegoś potrzebny?

Fazy życia feedbacku: koszt os./h

I. Review kodu + spisanie 2-3II. Weryfikacja 1-2III. Publikacja i uwagi od zespołu 1IV. Przepracowanie w zespole 0

Jak powstaje feedback

Faza I. Review kodu + spisanie uwag

￫ Keeper klonuje repo i ogląda kod

￫ Nie uruchamiamy tooli do statycznej analizy

￫ Keeper tworzy dokument feedbacku i proponuje ocenę

Jak powstaje feedback

Faza II. Weryfikacja

￫ Drugi Keeper przegląda feedback (kontrola merytoryczna)

￫ Przy rozbieżnościach - uzgadniamy w ramach CQK

￫ Kontrola czy feedback jest friendly

￫ Drugi Keeper często dopisuje swoje uwagi

￫ Approve

Jak powstaje feedback

Faza III. Publikacja i uwagi od zespołu

￫ Feedback jest przekazywany do zespołu

￫ Zachęcamy zespół do ustosunkowania się do feedback’u

Czy modyfikujemy feedback?

￫ Świadome i uzasadnione naruszenia reguł

￫ My czegoś nie zrozumieliśmy (brak kontekstu)

Jak powstaje feedback

Faza IV. Przepracowanie feedbacku w zespole

Mamy nadzieję, że zespół przekona się do naszych uwag i propozycji

Jak powstaje feedback

Co znaleźliśmy - big picture

Co znaleźliśmy - big picture

Główne grzechy:

￫ TDD - Test Driven Development

￫ Anemic Domain Model

Co znaleźliśmy - big picture

TDD - problemy:

￫ Overmocking￫ Mocking What-You-Don’t-Own￫ Testy są kopią implementacji￫ Za mało testów integracyjnych￫ Testy pisane pod Coverage￫ Testy bez wartości dokumentacyjnej￫ Testowanie implementacji a nie funkcjonalności

Co znaleźliśmy - big picture

Anemic Domain Model

￫ Anemiczne, mutowalne Encje￫ Settery i gettery rządzą￫ Programowanie proceduralne nie obiektowe￫ Często w ogóle brak wydzielonego Modelu Domenowego

Co znaleźliśmy - big picture

Test unitowy prostego konstruktora

Co znaleźliśmy - ‘perełki’

public class ClientException extends RuntimeException {

static final long serialVersionUID = -712897190232112939L;

public ClientException(String message) {

super(message);

}

public ClientException(String message, Throwable cause) {

super(message, cause);

}

}

src

@Test

public void shouldSetMessageAndCauseInConstructor() {

//given

String exceptedMessage = "Very bad exception";

Exception expectedCauseException = new Exception("This is cause exception");

//when

ClientException exception = new ClientException(exceptedMessage, expectedCauseException);

String message = exception.getMessage();

Throwable causeException = exception.getCause();

//then

assertEquals(exceptedMessage, message);

assertEquals(expectedCauseException, causeException);

}

test

Nie testuj unitowo prostych konstruktorów oraz

metod

CQK radzi

Test unitowy Serwisu-przelotki

Co znaleźliśmy - ‘perełki’

public class SaleRegulationsService {

public List<SaleRegulations> findRegulations(String userId, Long timestamp) {

List<SaleRegulations> results;

if (timestamp == null) {

results = salesRegulationsRepository.findOneByUserId(userId);

} else {

results = salesRegulationsRepository.findByUserId(userId);

}

return results;

}

...

}

src

@Test

public void shouldReturnMoreResultsWhenTimestampIsGiven() {

// given

SaleRegulationsService saleRegulationsService = new SaleRegulationsService(salesRegulationsRepository, eventProducer);

SaleRegulations saleRegulations1 = new SaleRegulations(USER_ID, SALE_REGULATIONS_TEXT, TIMESTAMP);

SaleRegulations saleRegulations2 = new SaleRegulations(USER_ID, SALE_REGULATIONS_TEXT_VERSION_2, TIMESTAMP2);

List<SaleRegulations> results = new ArrayList<>();

results.add(saleRegulations1);

results.add(saleRegulations2);

when(salesRegulationsRepository.findByUserId(USER_ID)).thenReturn(results);

// when

List<SaleRegulations> regulations = saleRegulationsService.findRegulations(USER_ID, TIMESTAMP);

// then

assertThat(regulations.size()).isEqualTo(2);

assertThat(regulations.get(0)).isEqualTo(saleRegulations1);

...

test

Nie testuj unitowo metod typu przelotka.

Testuj integracyjnie, używając Embedded (lub Fake) DB

CQK radzi

Overmocking

Co znaleźliśmy - ‘perełki’

@RunWith(MockitoJUnitRunner. class)

public class CreateEndpointTest {

@Mock

private RefundsSoapBindingStub npRefundService;

@Mock

private RefundsClient refundsClient;

@Mock

private PayuOrders payuOrders;

@Mock

private SignatureFactory signatureFactory;

@Mock

private RefundInfo refundInfoMock;

...

test

unikaj Mocków z verify(), to betonowanie kodu

CQK radzi

Konkurs Allegro

Kod: 5mSG5t

Najczęstsze błędy

Wstrzykiwanie zależności: pola vs konstruktor:

￫ Jasna deklaracja kolaboratorów

￫ Rozróżnienie opcjonalnych zależności

￫ Łatwiejsza konstrukcja obiektów w testach

Najczęstsze błędy

Niemutowalność:

“Classes should be immutable unless there's a very good

reason to make them mutable....If a class cannot be made

immutable, limit its mutability as much as possible.”

Joshua Bloch, Effective Java

Najczęstsze błędy

Mockowanie nie swoich rzeczy:(a.k.a: Don’t mock what you don’t own)

￫ zmiany poza naszą kontrolą

￫ podwójnie kosztowne (test / prod)

￫ przekonaliśmy zespoły do zewnętrznego review

￫ konwencja: should + given/when/then

￫ przekonaliśmy większość zespołów do Spocka

￫ zespoły piszą coraz lepsze testy integracyjne

￫ Overmockingu mniej

￫ zdecydowanie widać poprawę ogólnej jakości kodu

Sukcesy

￫ review w ramach jednego zespołu często nie wystarcza

￫ potrzeba systematyczności - jednorazowy zryw to za mało

￫ otwartość na feedback

￫ cierpliwość - efekty nie przyjdą od razu

Wnioski

Q/A?

Znajdziesz nas:Blog: allegrotech.io

Twitter: @allegrotechblog

pracuj z namikariera.allegro.pl