Hannes Schneidermayer Java- & webontwikkelaar

Mijn best practices – Algemeen

1. Commentaar – vloek en zegen

Goed commentaar is een kunst. Weloverwogen een gunst, maar vaak ook verspilde moeite.

Zelfdocumenterende code betekent niet “geen commentaar”. De code moet het hoe zo veel mogelijk zelf uitdrukken; commentaar vult het waarom aan, net als niet-voor-de-hand-liggende randvoorwaarden en het contract van een API.

Hieronder enkele voorbeelden van zelfdocumenterende code en van goed en slecht commentaar.

Een veelgehoord argument is dat commentaar snel verouderd raakt. Maar dat probleem geldt net zo goed voor namen van variabelen en methoden!

Au – doe dit nooit:

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

Zelfdocumenterend: goede variabelenamen zijn het halve werk

Slecht:

var dateTime = DateTimeUtil.getNow();

Goed:

var now = DateTimeUtil.getNow();

Nog een voorbeeld:

Slecht:

User user = this.getBruce();

Slecht:

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

Goed:

User userWithoutPermission = this.getBruce();

Zelfdocumenterend: goede namen voor methoden

Slecht:

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

Goed:

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

Zelfdocumenterend: private methoden

Slecht: de methode dwingt de lezer om validatie, berekening en bedrijfsregel tegelijk te ontcijferen.

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

Goed: de publieke methode vertelt het verloop. Private, stateless methoden benoemen elk afzonderlijk doel en maken hun afhankelijkheden zichtbaar via parameters en returnwaarden.

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

Niet elke regel heeft een eigen methode nodig. Extraheer een samenhangende gedachte die je precies kunt benoemen. Namen als processData() of handleStuff() leveren daarentegen niets op.

Vuistregel: becommentarieer waarom iets gebeurt, niet wat er gebeurt

Het is duidelijk wat hier gebeurt:

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

Maar waarom gebeurt het?

Beter:

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

Nog een voorbeeld uit de praktijk. Hier zie je methoden om de Ajax-laadspinner te tonen en te verbergen. Alleen zijn het er vier, en waarom is op het eerste gezicht niet helemaal duidelijk:

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

De methoden lijken sterk op elkaar en manipuleren allemaal op de een of andere manier de zichtbaarheid via CSS. Met een kort commentaar zijn de use cases meteen duidelijk:

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

Nog een voorbeeld:

Slecht:

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

Goed:

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

Discussie in de workshop van 2021-06-21: alle ontwikkelaars waren het erover eens dat Javadoc in het algemeen te vermijden is (parameter-nachtmerrie, voortdurend verouderd en niet prettig te onderhouden) en alleen gebruikt moet worden als een parameter echt extra uitleg nodig heeft, of als je anders de hele, lange methode zou moeten lezen.

Uitzondering op de vuistregel: becommentarieer wat er gebeurt – hier is het verstandig

Als het de leesbaarheid duidelijk verbetert, omdat de code anders onduidelijk is:

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

Commentaar – scheldwoorden in de code van de Linux-kernel

Tot slot nog een grappige anekdote uit de Linux-kernel – het aantal scheldwoorden in de broncode:

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

Veel plezier met het schrijven van commentaar!

2. Private methoden in grote routines zijn gevaarlijk

Let op! In de huidige objectgeoriënteerde wereld zijn er altijd veel private methoden. Normaal gesproken is dat geen probleem. Het wordt er pas een als die methoden velden, oftewel de state, benaderen en deel uitmaken van grote routines. Zulke submethoden horen private en stateless te zijn. Anders hebben ze het nadeel dat ze, los aangeroepen, vaak voor problemen zorgen. Ontwikkelaars komen namelijk op het idee om de submethode aan te roepen. Ongeldige states zijn een van de belangrijkste oorzaken van bugs. Een interessant artikel hierover, met enkele uitspraken van John Carmack (id Software, Doom), legt uit dat in toepassingen waar de code zeer stabiel moet zijn (bijvoorbeeld ruimtevaart) subroutines vaak verboden zijn, wat de code deterministischer maakt: http://number-none.com/blow/john_carmack_on_inlined_code.html

Deze anekdote is maar beperkt van toepassing op de moderne, stateless Java/Spring-wereld, maar is toch een goede denkoefening.