Hannes Schneidermayer Java- & webbutvecklare

Mina best practices – allmänt

1. Kommentarer – förbannelse och välsignelse

Bra kommentarer är en konst. Med eftertanke en gunst – men ofta utan vinst.

Självdokumenterande kod betyder inte ”inga kommentarer”. Koden ska så långt det går själv uttrycka hur; kommentarerna kompletterar med varför, med icke uppenbara randvillkor och med API:ets kontrakt.

Här följer några exempel på självdokumenterande kod samt bra och dåliga kommentarer.

Ett vanligt argument är att kommentarer lätt blir inaktuella. Men det problemet gäller faktiskt lika mycket för variabel- och metodnamn!

Aj – gör aldrig så här:

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

Självdokumenterande: bra variabelnamn är halva jobbet

Dåligt:

var dateTime = DateTimeUtil.getNow();

Bra:

var now = DateTimeUtil.getNow();

Ett annat exempel:

Dåligt:

User user = this.getBruce();

Dåligt:

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

Bra:

User userWithoutPermission = this.getBruce();

Självdokumenterande: bra metodnamn

Dåligt:

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

Bra:

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

Självdokumenterande: privata metoder

Dåligt: Metoden tvingar läsaren att tyda validering, beräkning och affärsregel på samma gång.

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

Bra: Den publika metoden berättar flödet. Privata, tillståndslösa metoder namnger de enskilda avsikterna och gör sina beroenden synliga via parametrar och returvärden.

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

Alla rader behöver inte en egen metod. Det som bryts ut är en sammanhängande tanke som går att namnge precist. Namn som processData() eller handleStuff() vinner däremot ingenting.

Tumregel: Kommentera varför något händer, inte vad som händer

Det är tydligt vad som händer här:

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

Men varför händer det?

Bättre:

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

Ännu ett exempel från verkligheten. Här visas metoder som visar och döljer Ajax-laddningssnurran. Men det finns fyra metoder, och varför är inte helt uppenbart vid första anblick:

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

Metoderna verkar vara väldigt lika varandra och på något sätt ändra synligheten via CSS. Med en kort kommentar blir användningsfallen genast tydliga:

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

Ett annat exempel:

Dåligt:

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

Bra:

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

Diskussion på workshoppen 2021-06-21: Samtliga utvecklare var överens om att Javadoc i regel bör undvikas (parametermardröm, ständigt inaktuellt och besvärligt att underhålla) och bara användas när en parameter verkligen behöver förklaras närmare, eller när man annars skulle behöva läsa hela den långa metoden.

Undantag från tumregeln: Kommentera vad som händer – här är det klokt

När det gör koden betydligt lättare att läsa, eftersom den annars inte är tydlig:

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

Kommentarer – svordomar i Linuxkärnans kod

Till sist en rolig historia från Linuxkärnan – antalet svordomar i källkoden:

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

Ha kul när du skriver kommentarer!

2. Privata metoder i stora programrutiner är farliga

Se upp! I dagens objektorienterade värld finns det alltid många privata metoder. Det är normalt inget problem. Det blir ett först när metoderna läser och skriver fält, alltså tillståndet (state), och ingår i stora rutiner. Sådana delmetoder bör vara privata och tillståndslösa. Annars har de nackdelen att de ofta ställer till det när de anropas fristående. Förr eller senare kommer någon utvecklare att få för sig att anropa delmetoden direkt. Otillåtna tillstånd är en av de främsta orsakerna till buggar. En intressant artikel om detta tar upp några uttalanden av John Carmack (id Software, Doom) och att subrutiner ofta är förbjudna i tillämpningar där koden måste vara extremt stabil (t.ex. rymdfart), vilket gör koden mer deterministisk: http://number-none.com/blow/john_carmack_on_inlined_code.html

Den här historien går bara delvis att tillämpa på dagens tillståndslösa Java/Spring-värld, men den är ändå en bra tankeställare.