Hannes Schneidermayer Programista Java & Web

Moje dobre praktyki – ogólne

1. Komentarze – przekleństwo i błogosławieństwo

Dobre komentarze to sztuka. Przemyślane są bezcenne, a jednak często nikt ich nie szuka.

Samodokumentujący się kod nie oznacza „zero komentarzy”. Kod powinien w miarę możliwości sam wyrażać jak; komentarze uzupełniają dlaczego, nieoczywiste warunki brzegowe i kontrakt API.

Poniżej kilka przykładów samodokumentującego się kodu oraz dobrych i złych komentarzy.

Częsty argument brzmi: komentarze szybko się dezaktualizują. Tak naprawdę ten problem dotyczy w tym samym stopniu nazw zmiennych i metod!

Auć – nigdy tak nie rób:

int age = 32; // set the value of the age integer to 32

Samodokumentujący się kod: sensowne nazwy zmiennych to połowa sukcesu

Źle:

var dateTime = DateTimeUtil.getNow();

Dobrze:

var now = DateTimeUtil.getNow();

Kolejny przykład:

Źle:

User user = this.getBruce();

Źle:

// testing user without permission
User user = this.getBruce();

Dobrze:

User userWithoutPermission = this.getBruce();

Samodokumentujący się kod: sensowne nazwy metod

Źle:

public String sanitizeInput(String input) {
    // replace underscore with spaces
    return input.replaceAll("_", " ");
}

Dobrze:

public String replaceUnderscoreWithSpaces(String input) {
    return input.replaceAll("_", " ");
}

Samodokumentujący się kod: metody prywatne

Źle: metoda zmusza czytelnika, by jednocześnie rozszyfrował walidację, obliczenia i regułę biznesową.

public Invoice createInvoice(Order order, Customer customer) {
    if (order.items().isEmpty()) {
        throw new IllegalArgumentException("Order needs at least one item");
    }

    BigDecimal subtotal = order.items().stream()
            .map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
            .reduce(BigDecimal.ZERO, BigDecimal::add);
    BigDecimal discount = customer.isPremium()
            ? subtotal.multiply(new BigDecimal("0.10"))
            : BigDecimal.ZERO;

    return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}

Dobrze: metoda publiczna opowiada przebieg. Prywatne, bezstanowe metody nazywają poszczególne intencje, a ich zależności są widoczne w parametrach i wartościach zwracanych.

public Invoice createInvoice(Order order, Customer customer) {
    requireItems(order);
    BigDecimal subtotal = calculateSubtotal(order.items());
    BigDecimal discount = loyaltyDiscountFor(customer, subtotal);

    return invoiceFor(order, subtotal, discount);
}

private static void requireItems(Order order) {
    if (order.items().isEmpty()) {
        throw new IllegalArgumentException("Order needs at least one item");
    }
}

private static BigDecimal calculateSubtotal(List<OrderItem> items) {
    return items.stream()
            .map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
            .reduce(BigDecimal.ZERO, BigDecimal::add);
}

private static BigDecimal loyaltyDiscountFor(Customer customer, BigDecimal subtotal) {
    return customer.isPremium()
            ? subtotal.multiply(new BigDecimal("0.10"))
            : BigDecimal.ZERO;
}

private static Invoice invoiceFor(
        Order order,
        BigDecimal subtotal,
        BigDecimal discount
) {
    return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}

Nie każda linijka potrzebuje własnej metody. Wydziela się spójną myśl, którą da się precyzyjnie nazwać. Nazwy w rodzaju processData() czy handleStuff() niczego natomiast nie wnoszą.

Praktyczna zasada: komentuj, dlaczego coś się dzieje, a nie co się dzieje

Jest jasne, co się tu dzieje:

public Duration retryDelay(int retryCount) {
    long seconds = Math.min(1L << retryCount, 60);
    return Duration.ofSeconds(seconds);
}

Ale dlaczego?

Lepiej:

public Duration retryDelay(int retryCount) {
    // Cap the delay so an outage never blocks queued jobs for more than one minute.
    long seconds = Math.min(1L << retryCount, 60);
    return Duration.ofSeconds(seconds);
}

Kolejny przykład z życia. Widzimy metody, które pokazują i ukrywają spinner ładowania Ajaxa. Tyle że metody są cztery. Dlaczego – na pierwszy rzut oka nie jest to jasne:

function showAjaxLoader() {
    $('.layout-ajax-loader').children().first().show();
}

function hideAjaxLoader() {
    $('.layout-ajax-loader').children().first().hide();
}

function suppressAjaxLoader() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
}

function releaseAjaxLoader() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
}

Wszystkie metody wydają się bardzo podobne i jakoś manipulują widocznością przez CSS. Krótki komentarz od razu wyjaśnia, do czego służą:

showAjaxLoader: function() {
    $('.layout-ajax-loader').children().first().show();
},

hideAjaxLoader: function() {
    $('.layout-ajax-loader').children().first().hide();
},

// take the control from primefaces and don't allow the spinner to show, no matter what
suppressAjaxLoader: function() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
},

// reset the state after hiding, to give the control back to primefaces
releaseAjaxLoader: function() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
},

Kolejny przykład:

Źle:

/**
 * Sets the tool tip text.
 *
 * @param text  the text of the tool tip
 */
public void setToolTipText(String text) {}

Dobrze:

/**
 * Registers the text to display in a tool tip. The text
 * displays when the cursor lingers over the component.
 *
 * @param text  the string to display. If the text is null,
 *              the tool tip is turned off for this component.
 */
public void setToolTipText(String text) {}

Dyskusja na warsztatach z 21.06.2021: Wszyscy programiści zgodzili się, że Javadoca należy co do zasady unikać (koszmar z parametrami, ciągle nieaktualny i niewygodny w utrzymaniu) i stosować go tylko wtedy, gdy parametr naprawdę wymaga dokładniejszego wyjaśnienia albo gdy inaczej trzeba by przeczytać całą długą metodę.

Wyjątek od reguły: komentuj, co się dzieje – tutaj to wskazane

Gdy znacznie poprawia to czytelność, bo inaczej kod nie byłby jasny:

let isAlwaysAllowedKey = (
    e.which === 8  || /* BACKSPACE */
    e.which === 35 || /* END */
    e.which === 36 || /* HOME */
    e.which === 37 || /* LEFT */
    e.which === 38 || /* UP */
    e.which === 39 || /* RIGHT*/
    e.which === 40 || /* DOWN */
    e.which === 46 || /* DEL*/
    e.ctrlKey === true && e.which === 65 || /* CTRL + A */
    e.ctrlKey === true && e.which === 88 || /* CTRL + X */
    e.ctrlKey === true && e.which === 67 || /* CTRL + C */
    e.ctrlKey === true && e.which === 90   /* CTRL + Z */
)

Komentarze – przekleństwa w kodzie jądra Linuksa

Na koniec zabawna ciekawostka z jądra Linuksa – liczba wszystkich przekleństw w kodzie źródłowym:

https://www.vidarholen.net/contents/wordcount/#fuck*,shit*,damn*,idiot*,retard*

Miłego pisania komentarzy!

2. Metody prywatne w dużych procedurach są niebezpieczne

Uwaga! W dzisiejszym obiektowym świecie metod prywatnych jest zawsze mnóstwo. Zwykle to żaden problem. Staje się nim dopiero wtedy, gdy te metody sięgają do pól, czyli do stanu, i są częścią dużych procedur. Takie metody pomocnicze powinny być prywatne i bezstanowe. W przeciwnym razie wywołane samodzielnie często narobią kłopotów. Programiści prędzej czy później wpadną na pomysł, żeby wywołać taką metodę pomocniczą osobno. Niedozwolone stany to jedna z głównych przyczyn bugów. Ciekawy artykuł na ten temat o kilku wypowiedziach Johna Carmacka (id Software, Doom) i o tym, że w aplikacjach, których kod musi być wyjątkowo stabilny (np. w lotach kosmicznych), podprocedury są często zakazane, co czyni kod bardziej deterministycznym: http://number-none.com/blow/john_carmack_on_inlined_code.html

Ta anegdota tylko częściowo przekłada się na nowoczesny, bezstanowy świat Javy i Springa, ale i tak daje do myślenia.