Hannes Schneidermayer Desarrollador Java y web

Mis buenas prácticas – General

1. Comentarios: maldición y bendición

Los buenos comentarios son un arte. Bien pensados, un regalo; pero a menudo, esfuerzo en vano.

El código autodocumentado no significa «sin comentarios». El código debe expresar el cómo por sí mismo en la medida de lo posible; los comentarios añaden el porqué, las condiciones que no son evidentes y el contrato de una API.

A continuación, algunos casos de código autodocumentado, así como de comentarios buenos y malos.

Un argumento habitual es que los comentarios se quedan desactualizados con facilidad. ¡En realidad, ese problema afecta igual a los nombres de variables y métodos!

Ay. Nunca hagas esto:

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

Autodocumentado: unos buenos nombres de variables son media batalla ganada

Mal:

var dateTime = DateTimeUtil.getNow();

Bien:

var now = DateTimeUtil.getNow();

Otro ejemplo:

Mal:

User user = this.getBruce();

Mal:

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

Bien:

User userWithoutPermission = this.getBruce();

Autodocumentado: nombres de métodos con sentido

Mal:

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

Bien:

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

Autodocumentado: métodos privados

Mal: el método obliga al lector a descifrar a la vez la validación, el cálculo y la regla de negocio.

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

Bien: el método público cuenta el flujo. Los métodos privados y sin estado dan nombre a cada intención y hacen visibles sus dependencias a través de parámetros y valores de retorno.

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

No todas las líneas necesitan su propio método. Lo que se extrae es una idea cohesionada a la que se le puede poner un nombre preciso. Nombres como processData() o handleStuff(), en cambio, no aportan nada.

Regla general: comenta por qué ocurre algo, no qué ocurre

Está claro qué ocurre aquí:

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

Pero ¿por qué ocurre?

Mejor:

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

Otro ejemplo de la vida real. Se muestran métodos para mostrar y ocultar el spinner de carga de Ajax. Sin embargo, hay cuatro métodos, y a primera vista no queda del todo claro por qué:

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

Todos los métodos parecen muy similares y, de alguna manera, manipulan la visibilidad mediante CSS. Con un breve comentario, los casos de uso quedan claros al instante:

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

Otro ejemplo:

Mal:

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

Bien:

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

Debate en el taller del 2021-06-21: Opinión compartida por todos los desarrolladores: en general, conviene evitar el Javadoc (una pesadilla de parámetros, siempre desactualizado y poco agradable de mantener) y usarlo solo cuando un parámetro necesita de verdad una explicación más detallada o cuando, de lo contrario, habría que leerse todo el método, largo, entero.

Excepción a la regla general: comenta qué ocurre; aquí es recomendable

Cuando facilita mucho la lectura, porque de otro modo el código no queda claro:

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

Comentarios: palabrotas en el código del kernel de Linux

Para terminar, una anécdota divertida del kernel de Linux: el número total de palabrotas en el código fuente:

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

¡Que disfrutes escribiendo comentarios!

2. Los métodos privados en rutinas grandes son peligrosos

¡Atención! En el mundo orientado a objetos actual siempre hay muchos métodos privados. Normalmente, eso no es un problema. Solo lo es cuando esos métodos acceden a campos, es decir, al estado, y forman parte de rutinas grandes. Este tipo de submétodos deberían ser privados y sin estado. De lo contrario, tienen la desventaja de que, si se llaman por separado, a menudo dan problemas. A algún desarrollador se le ocurrirá llamar al submétodo. Los estados ilegales son una de las principales causas de bugs. Un artículo interesante al respecto, sobre algunas afirmaciones de John Carmack (id Software, Doom), explica que en aplicaciones donde el código tiene que ser muy estable (los vuelos espaciales, por ejemplo) a menudo se prohíben las subrutinas, lo que hace el código más determinista: http://number-none.com/blow/john_carmack_on_inlined_code.html

Esta anécdota solo se aplica en parte al mundo moderno y sin estado de Java/Spring, pero aun así da que pensar.