TL;DR
In my opinion, dealing with logic explicitly is better than implicitly. Hiding logic is a bad programming practise.
The Story
A user will input some search criteria, and our program will validate the input against some certain rules.
These's already a validation method, and all we need to do is to extend it.
// Original code
void validate(SearchArgs args) {
List<String> errors;
validationSomething(args, errors);
validationSomethingElse(args, errors);
// we need add another validation here
if (!errors.isEmpty()) {
throw new ValidationError(args, errors);
}
}
According to the requirement, there will be one more error message generated if the user input fails our new validation rule.
So we make it straight forward:
Optional<String> validateNewRule(SearchArgs args) {
// TODO:
}
// And in validate()
validateNewRule(args).ifPresent(error -> {
errors.add(error);
});
Writing test cases, test case passes, build pipeline passes...
During the code review, another dev raised an idea: shouldn't our new validateNewRule() method return a List of errors?
List<String> validateNewRule(SearchArgs args) {
// TODO:
}
// And in validate()
errors.addAll(validateNewRule(args));
The Thoughts
I'm not a fan of doing such implementation style argues in a code review, but since somebody raised this, I'd like to list the Pros and Cons of each approach.
The Optional<String> way
Pros
- The output is explicit, an optional value indicates if there's an error
Cons (I doubt this)
- Explicit, the method consumer will have logic to tell if there's an error and whether it needs to add the error into error list
The List<String> way
Pros
- No branch (
if
, or something similar) - Possible future proof if the requirement changes and validateNewRule() needs to return a list of errors
Cons
- If there's no change to the requirement, the effort of returning a list is wasted
- Implicit
I don't want to discuss if the 'future proof' is necessary, but I'd like to talk more about why I believe it's implicit and why it's bad.
Nowadays many programmers believe adding more branches increases the program's complexity.
I agree.
But they're going too far. They've started hiding logics.
Let's think about this, by using a list of errors, we presume:
a) Returning an empty list means no errors
b) Returning a non-empty list means validation fails
See? There's a branch.
There's no if
in the code only because we have above assumptions, and the logic is implicitly hidden in the assumptions.
This sounds great in this given example, but if you think about reusing this validateNewRule() in a scenario where it's solely used:
List<String> errors = validateNewRule(args);
if (!errors.isEmpty()) {
// handle errors
}
It's perfectly correct to say, it's very easy to make mistake by forget typing the logic NOT operator.
While the Optional<String>, and explicit version:
Optional<String> error = validateNewRule(args);
if (error.isPresent()) {
// handle error
}
validateNewRule(args).ifPresent(error -> {
// handle error
});
validateNewRule(args)
.map(...); // handle error
No one's gonna make mistake on all three types of implementations.
I hope this article clearly expresses my idea.
Thoughts?
I care about creating code hard to make mistakes and easy to replace.
I don't focus on creating 'easy to understand' code - it's a programmer's basic capability.
网友评论