1
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?
Sorry for the delay in responding. "Choose to work with less specific types whenever possible" seems like a good tip, I’m curious to know where/how you learned it. As for calling a method several times, what would be the harm in that? Would be indicative of flawed logic? Its in
parsePositiveInt()
does not take into account that a time may benull
and that it is better that the method does not launch exception (in my view an invalid user input is not so exceptional, it is almost expected). As for desktop MVC, it is desirable, but I’m not sure where to start applying it in the application.– Piovezan
Hello, I saw you did not come back to answer my comment, I would really like to apologize for the delay in commenting your reply because I took the time to reflect on it carefully because it had a lot of good content. I apologize again and thank you for taking the time to answer my question.
– Piovezan
@Piovezan I answered your questions, let me know if you want me to expand some explanation.
– Gabriel
Thank you so much for the answers. It cost a little but I understood the question of the less specific parameters;
System.out.println()
is quite reusable, so should be my methods. I disagree with the performance issue in this particular case because the parsing performance is laughable. On the possibility of a parameter comingnull
, I don’t think I was clear, butnull
is a valid value, it should not cast exception in this case.– Piovezan