Hannes Schneidermayer Sviluppatore Java & Web

Le mie best practice – Generale

1. Commenti: croce e delizia

Scrivere buoni commenti è un’arte. Ben ponderati sono un dono, ma spesso restano un vuoto suono.

Codice autodocumentante non significa «niente commenti». Il codice dovrebbe esprimere da sé il più possibile il come; i commenti aggiungono il perché, i vincoli non ovvi e il contratto di un’API.

Seguono alcuni casi di codice autodocumentante, oltre a esempi di commenti buoni e cattivi.

Un argomento frequente è che i commenti tendono a diventare obsoleti. In realtà questo problema vale allo stesso modo per i nomi di variabili e metodi!

Ahi – non farlo mai:

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

Autodocumentazione: nomi sensati per le variabili sono già metà dell’opera

Sbagliato:

var dateTime = DateTimeUtil.getNow();

Corretto:

var now = DateTimeUtil.getNow();

Un altro esempio:

Sbagliato:

User user = this.getBruce();

Sbagliato:

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

Corretto:

User userWithoutPermission = this.getBruce();

Autodocumentazione: nomi sensati per i metodi

Sbagliato:

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

Corretto:

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

Autodocumentazione: metodi privati

Sbagliato: il metodo costringe chi legge a decifrare contemporaneamente validazione, calcolo e regola di business.

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));
}

Corretto: il metodo pubblico racconta il flusso. Metodi privati e stateless danno un nome ai singoli intenti e rendono visibili le loro dipendenze tramite parametri e valori di ritorno.

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));
}

Non ogni riga ha bisogno di un metodo tutto suo. Si estrae un pensiero coerente a cui si può dare un nome preciso. Nomi come processData() o handleStuff(), invece, non aggiungono nulla.

Regola pratica: commenta perché succede qualcosa, non che cosa succede

È chiaro che cosa succede qui:

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

Ma perché succede?

Meglio:

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);
}

Un altro esempio dalla vita reale. Ecco i metodi per mostrare e nascondere lo spinner di caricamento Ajax. Solo che i metodi sono quattro, e il perché non è del tutto chiaro a prima vista:

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 = '';
}

I metodi sembrano tutti molto simili e in qualche modo manipolano la visibilità tramite CSS. Con un breve commento i casi d’uso diventano subito chiari:

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 = '';
},

Un altro esempio:

Sbagliato:

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

Corretto:

/**
 * 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) {}

Discussione nel workshop del 21 giugno 2021: tutti gli sviluppatori hanno condiviso l’opinione che il Javadoc vada in generale evitato (incubo dei parametri, sempre obsoleto e poco piacevole da mantenere) e che vada usato solo quando un parametro ha davvero bisogno di una spiegazione più dettagliata o quando altrimenti si dovrebbe leggere l’intero, lungo metodo.

Eccezione alla regola pratica: commentare che cosa succede, qui è consigliabile

Quando semplifica nettamente la lettura, perché altrimenti il codice non sarebbe chiaro:

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 */
)

Commenti: le parolacce nel codice del kernel Linux

Per finire, una chicca divertente dal kernel Linux: il numero di tutte le parolacce nel codice sorgente:

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

Buon divertimento con i commenti!

2. I metodi privati nelle routine lunghe sono pericolosi

Attenzione! Nel mondo orientato agli oggetti di oggi ci sono sempre molti metodi privati. Di solito non è un problema. Lo diventa quando questi metodi accedono ai campi, cioè allo stato, e si trovano all’interno di routine lunghe. Questi sotto-metodi dovrebbero essere privati e stateless. Altrimenti hanno lo svantaggio che, chiamati da soli, spesso creano guai. Prima o poi a qualche sviluppatore verrà l’idea di chiamare il sotto-metodo. Gli stati illegali sono una delle cause principali dei bug. Un articolo interessante a riguardo, su alcune affermazioni di John Carmack (id Software, Doom), spiega che nelle applicazioni in cui il codice deve essere estremamente stabile (per esempio nei voli spaziali) le subroutine sono spesso vietate, il che rende il codice più deterministico: http://number-none.com/blow/john_carmack_on_inlined_code.html

Questa storia si applica solo in parte al moderno mondo stateless di Java/Spring, ma resta un buon spunto di riflessione.