Hannes Schneidermayer Java & Web Developer

Meine Best Practices – Allgemein

1. Kommentare – Fluch und Segen

Gute Kommentare sind eine Kunst. Bedacht eine Gunst, doch auch oft umsonst.

Self-documenting Code bedeutet nicht „keine Kommentare“. Der Code soll das Wie möglichst selbst ausdrücken; Kommentare ergänzen das Warum, nicht offensichtliche Randbedingungen und den Vertrag einer API.

Nachfolgend einige Fälle für self-documenting Code, sowie gute und schlechte Kommentare.

Ein häufiges Argument ist, dass Kommentare gerne outdated werden. Tatsächlich gilt diese Problematik für Variablen- und Methodennamen gleichermaßen!

Ouch – Never do this:

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

Self-documenting: Sinnvolle Namen für Variablen sind die halbe Miete

Bad:

var dateTime = DateTimeUtil.getNow();

Good:

var now = DateTimeUtil.getNow();

Ein weiteres Beispiel:

Bad:

User user = this.getBruce();

Bad:

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

Good:

User userWithoutPermission = this.getBruce();

Self-documenting: Sinnvolle Namen für Methoden

Bad:

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

Good:

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

Self-documenting: Private Methoden

Bad: Die Methode zwingt den Leser, Validierung, Berechnung und Fachregel gleichzeitig zu entschlüsseln.

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

Good: Die öffentliche Methode erzählt den Ablauf. Private, stateless Methoden benennen die einzelnen Absichten und machen ihre Abhängigkeiten über Parameter und Rückgabewerte sichtbar.

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

Nicht jede Zeile braucht eine eigene Methode. Extrahiert wird ein zusammenhängender Gedanke, der sich präzise benennen lässt. Namen wie processData() oder handleStuff() gewinnen dagegen nichts.

Faustregel: Kommentiere warum etwas passiert, statt was passiert

Es ist klar, was hier passiert:

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

Aber warum passiert es?

Besser:

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

Ein weiteres real-life Beispiel. Gezeigt werden Methoden um den Ajax-Ladespinner ein- und auszublenden. Allerdings gibt es vier Methoden. Warum ist nicht ganz klar auf den ersten Blick:

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

Die Methoden scheinen alle sehr ähnlich zu sein und irgendwie die Sichtbarkeit via CSS zu manipulieren. Mit einem kurzen Kommentar werden die Anwendungsfälle direkt klar:

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

Ein weiteres Beispiel:

Bad:

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

Good:

/**
 * 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 im Workshop vom 2021-06-21: Geteilte Meinung aller Entwickler, dass Javadoc generell zu vermeiden ist (Parameter-Nightmare, ständig outdated und nicht angenehm wartbar) und nur Anwendung finden sollte, wenn ein Parameter wirklich nähere Erklärung braucht bzw. man sonst die ganze, lange Methode lesen müsste.

Ausnahme zur Faustregel: Kommentiere was passiert – hier ist es ratsam

Wenn es die Lesbarkeit deutlich vereinfacht, weil der Code sonst nicht klar ist:

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

Kommentare – Schimpfwörter im Code des Linux-Kernel

Abschließend noch ein lustiger Schwank aus dem Linux Kernel – die Anzahl aller Schimpfwörter im Source Code:

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

Have fun writing comments!

2. Private Methoden in grossen Programmroutinen sind gefährlich

Achtung! In der heutigen objektorientierten Welt gibt es immer viele private Methoden. Dies ist normalerweise kein Problem. Es wird erst zu einem, wenn diese Methoden auf Felder, sprich den State, zugreifen und in grossen Routinen sind. Solche Sub-Methoden sollten private und stateless sein. Sonst haben sie den Nachteil, dass alleine gecallt oft für Ärger sorgen können. Entwickler werden auf die Idee kommen, die Sub-Methode zu callen. Illegale States sind einer der Hauptgründe für Bugs. Interessanter Artikel hierzu über einige Aussagen von John Carmack (id Software, Doom) und dass in Anwendungen, wo der Code sehr stabil sein muss (Weltraumflug zB), oft Subroutinen verboten sind, was den Code deterministischer macht: http://number-none.com/blow/john_carmack_on_inlined_code.html

Dieser Schwank ist nur bedingt auf die moderne stateless Java/Spring-Welt anzuwenden, aber dennoch ein guter Denkanstoss.