Code review: field validations, how to abstract correctly?

Question

In a Java Swing application I have four JTextFields:

• Weather minimum of handover of an order in minutes
• Weather maximum of handover of an order in minutes
• Weather minimum of withdrawal of an order in minutes
• Weather maximum of withdrawal of an order in minutes

Delivery times are related to each other (minimum must be less than maximum) and the same for withdrawal times.

The user can fill these fields as he wants, with text or numbers, but what has value for me and what I expect him to fill are integer values equal to or greater than 0 (zero) or then null/empty (I cannot use the JSpinner for this because it does not accept null/empty values, only integers; empty field it treats as zero).

After filling the fields, the user presses a "Save" button that saves the values if they are valid or displays an error message if any of them is invalid (that is, if it is non-integer or less than zero). There are also additional validations that the minimum fields should be smaller than the maximum.

I spent a few hours with this code and it took the necessary abstractions to come out. I wanted to understand better how I should reason to get to these abstractions more quickly. I do not know how to break the requirements in minor functions, that do little.

I devised an algorithm to run when the "Save" button is pressed, but the implementation ended up not obeying it to the letter, and also did not leave with the division in methods I expected. That was the algorithm:

• Check if value is null/empty. If it is, the validation is finished. Otherwise, go to next validation.

• Try parsing value as integer. If unable to parse, display an error message, but skip to next validation.

• If the parsed value is less than zero, display an error message, otherwise it can go to the next validation.

• Validates if the minimum delivery time is less than the maximum, if it is not, displays error message. If it is, it switches to the next validation.

• Validates the minimum withdrawal time is less than the maximum, if it is not, it displays error message. If it is, the validations have passed and the application can continue with the rescue.

It seems simple and it seems that the algorithm is ready to implement, but I could not decide the signatures of the methods and the level of abstraction to which they should belong. I ended up getting a "spanking" of the code for a few hours. I could even try to solve it anyway, but I’m trying to be particularly strict with this part of the code, even for learning.

I wanted to understand which line of reasoning I should follow to turn these requirements into maintainable code and with good abstraction. One of my doubts is whether the principle CQS (Command-Query Separation) must be respected or not. In my case I considered that he was in the way and ignored him.

private static final int DELIVERY_AND_COUNTER_MINIMUM_TIME = 0;

private JTextField textFieldTempoDeEntregaMin;
private JTextField textFieldTempoDeEntregaMax;
private JTextField textFieldTempoDeRetiradaMin;
private JTextField textFieldTempoDeRetiradaMax;

private void jButtonSalvarActionPerformed(ActionEvent evt) {

    trimTextField(textFieldTempoDeEntregaMin);
    trimTextField(textFieldTempoDeEntregaMax);
    trimTextField(textFieldTempoDeRetiradaMin);
    trimTextField(textFieldTempoDeRetiradaMax);

    if (false == validateTime(textFieldTempoDeEntregaMin, "Tempo de entrega mínimo") ||
        false == validateTime(textFieldTempoDeEntregaMax, "Tempo de entrega máximo") ||
        false == validateTime(textFieldTempoDeRetiradaMin, "Tempo de retirada mínimo") ||
        false == validateTime(textFieldTempoDeRetiradaMax, "Tempo de retirada máximo")) {
        return;
    }

    if (false == isSmallerThan(textFieldTempoDeEntregaMin, textFieldTempoDeEntregaMax)) {
        showMessage("Tempo mínimo de entrega não pode ser maior ou igual que máximo.");
        return;
    }

    if (false == isSmallerThan(textFieldTempoDeRetiradaMin, textFieldTempoDeRetiradaMax)) {
        showMessage("Tempo mínimo de retirada não pode ser maior ou igual que máximo.");
        return;
    }

    Integer minDeliveryTimeInMinutes = parseValidTime(textFieldTempoDeEntregaMin.getText());
    Integer maxDeliveryTimeInMinutes = parseValidTime(textFieldTempoDeEntregaMax.getText());
    Integer minCounterTimeInMinutes = parseValidTime(textFieldTempoDeRetiradaMin.getText());
    Integer maxCounterTimeInMinutes = parseValidTime(textFieldTempoDeRetiradaMax.getText());

    DeliveryAndCounterTimes timesToChange = new DeliveryAndCounterTimes(minDeliveryTimeInMinutes,
            maxDeliveryTimeInMinutes, minCounterTimeInMinutes, maxCounterTimeInMinutes);

    // Efetua o salvamento.
}

private void trimTextField(JTextField textField) {
    textField.setText(textField.getText().trim());
}

private boolean validateTime(JTextField textField, String fieldName) {
    String text = textField.getText();
    if (isNotEmptyNorParsableToInteger(text)) {
        showMessage(fieldName + ": valor inválido (não é um número inteiro).");
        return false;
    } else if (isParsableToNegative(text)) {
        showMessage(fieldName + ": valor inválido (menor que zero).");
        return false;
    }

    // Is either empty or parsable to non-negative
    return true;
}

private boolean isParsableToNegative(String text) {
    return IntegerUtils.isParsable(text) && Integer.parseInt(text) < DELIVERY_AND_COUNTER_MINIMUM_TIME;
}

private boolean isNotEmptyNorParsableToInteger(String text) {
    return text != null && false == text.trim().isEmpty() && false == IntegerUtils.isParsable(text);
}

private void showMessage(String message) {
    JOptionPane.showMessageDialog(this, message);
}

private boolean isSmallerThan(JTextField textFieldMinimum, JTextField textFieldMaximum) {
    String textMinimum = textFieldMinimum.getText();
    String textMaximum = textFieldMaximum.getText();
    Integer minimum = parseValidTime(textMinimum);
    Integer maximum = parseValidTime(textMaximum);

    return (minimum != null && maximum != null && minimum < maximum);
}

private Integer parseValidTime(String validTime) {
    if (IntegerUtils.isParsable(validTime)) {
        if (Integer.parseInt(validTime) < DELIVERY_AND_COUNTER_MINIMUM_TIME) {
            throw new IllegalStateException("Tempo negativo: " + validTime);
        }
        return Integer.parseInt(validTime);
    } else if (validTime != null && false == validTime.isEmpty()) {
        throw new IllegalStateException("Tempo inválido: " + validTime);
    } else {
        return null;
    }
}

Integerutils.java:

public class IntegerUtils {

    public static boolean isParsable(String input) {
        boolean parsable = true;
        try {
            Integer.parseInt(input);
        } catch(NumberFormatException e) {
            parsable = false;
        }
        return parsable;
    }
}

I think this code size is ideal for a code review without the question getting too broad. But code review is not quite what I want; I look for ways to break the requirements in functions so that the break is close to this result without taking so long in other code reasoning that ended up being thrown away.

Observing the code developed, there are two moments that I consider keys:

• When it was decided to ignore CQS. This made it possible to display error messages as a side effect of some methods.
• When it was decided to deal only with non-price fields valid according to the business rule. This divided the development into two stages: before and after validating the fields.

Other moments can be perceived, but with less influence. I wonder if there is a very specific line of reasoning for the problem in question or more general principles that can be applied when trying to break the problem.

At first I wasted time trying to implement something like this:

private void jButtonSalvarActionPerformed(ActionEvent evt) {

    validateTimes();

    ...

}

private void validateTimes() {
    validateTime(textFieldTempoDeEsperaMin.getText());
    validateTime(textFieldTempoDeEsperaMax.getText());
    validateTime(textFieldTempoDeRetiradaMin.getText());
    validateTime(textFieldTempoDeRetiradaMax.getText());
}

Some findings that were needed:

• There are four text fields, so there will be four independent validations;
• To work properly validating pair by pair it is necessary to validate the fields individually first;
• Each validation is independent of the other; there is no similarity to be used, in particular in displaying error messages;
• Expect to have a good overview of the code, to reduce duplicates;
• The method isParsable() came into the implementation when I Googled about the behavior of parsing strings. It’s an interesting method and it comes in handy, but I probably wouldn’t have thought of it on my own. In another scenario what might have led me to think of him?

Answer 1

Nomenclature

You use names in English (textFieldTempoDeEntregaMin) and in English (isSmallerThan). It’s not a big deal, but it makes the project ugly. When I maintain a project that mixes styles of nomenclature, I get the impression that it was done "in the thighs".

Consistency in abstraction

There are auxiliary methods that work with less specific types, such as the isNotEmptyNorParsableToInteger, and others who work with very specific types, such as isSmallerThan. This is similar to code Smell called inappropriate intimacy. Choose to work with less specific types whenever possible, this will allow you to reuse these methods in many different contexts.

Efficiency and separation of responsibilities part 1

You call the method Integer.parseInt often. Let’s say the method parseValidTime is called with the parameter "42":

1. The first conditional calls the method isParsable, which, in turn, calls the Integer.parseInt.
2. The next parole calls directly the Integer.parseInt.
3. The return also calls the Integer.parseInt.

There were 3 calls in a row passing exactly the same argument! You can greatly improve this logic. Example:

private static int parsePositiveInt(String strValue) {
    int value = Integer.parseInt(strValue);

    if (value < 0) {
        throw new IllegalArgumentException();
    }

    return value;
}

Note that I have removed the references to the word "time" and the call to showMessage. The fact that this method is used to validate delivery time is totally irrelevant to the method itself. Now it has become reusable and you can apply it to any other chunk of code that needs to convert strings to positive integers. Example of use:

private void jButtonSalvarActionPerformed(ActionEvent evt) {
    int tempoMinimoEntrega;

    try {
        tempoMinimoEntrega = parsePositiveInt(textFieldTempoDeEntregaMin.getText());
    } catch (IllegalArgumentException ex) {
        showMessage("O tempo mínimo de entrega deve ser um número inteiro positivo");
        return;
    }

    // ...
}

In fact, the method parsePositiveInt should not be declared inside the screen controller.

Architecture and Separation of Responsibilities part 2

An eye-popping problem is that you have mixed up the business rules quite a bit with data presentation. In javax.Swing projects it is very common this happen, unfortunately. I strongly advise you to model your application using MVC (or other similar standard), just as we do in web applications. The only difference, in this case, would be that the view is not HTML code, but Java code.

The controller and view do not need to know that the minimum time should be less than the maximum time. This is a particularity of your business, not of the screen, so this should be done by a class specialized in validating business rules relating to delivery (for example, a class called EntregaService).

With this separation you can use a unit test framework to test the business rules in isolation, and make the code less confusing.

---

Answering questions raised in the comments

"Choose to work with less specific types whenever possible" seems a good tip, I’m curious to know where/how you learned it

From personal experience and because it is something natural. Class hierarchy exists for a reason: Abstraction. Imagine if the method System.out.println would not accept a Object, you could not use it to print objects from classes you created. Due to the fact that all classes inherit from Object, this method accepts any object.

As for calling a method several times, what would be the harm in that?

The biggest evil is the loss of performance. You could save the result of Integer.parseInt into a variable and use the variable instead of having Java convert the string again and again.

in parsePositiveInt() does not take into account that a time can be null and that it is better that the method does not throw exception (in my view an invalid user input is not so exceptional as it is almost expected)

The method Integer.parseInt spear IllegalArgumentException if the argument given is null. That means that the try-catch I did inside the jButtonSalvarActionPerformed you’ll be handling this case.