Hannes Schneidermayer Java & Web Developer

My Best Practices – General

1. Comments – a blessing and a curse

Good comments are an art. Used with care, they’re a gift – but all too often, they’re just hot air.

Self-documenting code doesn’t mean “no comments”. The code itself should express the how as far as possible; comments add the why, non-obvious constraints and the contract of an API.

Below are a few cases of self-documenting code, along with good and bad comments.

A common argument is that comments tend to go out of date. Truth is, the same applies to variable and method names!

Ouch – Never do this:

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

Self-documenting: meaningful variable names get you halfway there

Bad:

var dateTime = DateTimeUtil.getNow();

Good:

var now = DateTimeUtil.getNow();

Another example:

Bad:

User user = this.getBruce();

Bad:

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

Good:

User userWithoutPermission = this.getBruce();

Self-documenting: meaningful method names

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 methods

Bad: the method forces the reader to decipher validation, calculation and business rule all at once.

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: the public method tells the story. Private, stateless methods name each individual intent and make their dependencies visible through parameters and return values.

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

Not every line needs its own method. What you extract is a coherent thought that can be named precisely. Names like processData() or handleStuff(), on the other hand, gain you nothing.

Rule of thumb: comment why something happens, not what happens

It’s clear what is happening here:

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

But why is it happening?

Better:

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

Another real-life example. These methods show and hide the Ajax loading spinner. But there are four of them, and why isn’t quite clear at first glance:

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

The methods all look very similar and somehow manipulate visibility via CSS. A short comment makes the use cases clear right away:

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

Another example:

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

Discussion in the workshop on 2021-06-21: The developers shared the view that Javadoc should generally be avoided (parameter nightmare, constantly outdated and a pain to maintain) and only be used when a parameter really needs further explanation or you’d otherwise have to read the entire, long method.

Exception to the rule of thumb: comment what happens – here it’s a good idea

When it makes the code much easier to read, because it wouldn’t be clear otherwise:

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

Comments – swear words in the Linux kernel code

To wrap up, a funny anecdote from the Linux kernel – the count of all swear words in its source code:

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

Have fun writing comments!

2. Private methods in large routines are dangerous

Watch out! In today’s object-oriented world, there are always lots of private methods. That’s usually not a problem. It only becomes one when these methods access fields, i.e. the state, and live inside large routines. Such sub-methods should be private and stateless. Otherwise, calling them on their own can easily cause trouble – and developers will eventually get the idea to call the sub-method. Illegal states are one of the main causes of bugs. There’s an interesting article on this about some statements by John Carmack (id Software, Doom), and how applications where the code has to be extremely stable (spaceflight, for example) often ban subroutines altogether, which makes the code more deterministic: http://number-none.com/blow/john_carmack_on_inlined_code.html

This anecdote only partly applies to the modern stateless Java/Spring world, but it’s still good food for thought.