1

So, I recently added some validation logic to our controllers:

interface Validator<T> {
    fun validate(model: T): Boolean
}

Which is used in the controller like this:

validator.validate(request) || throw Exception()

During code review my colleague was thrown off by validate || throw construct, while to me it is succinct and communicates intent very clearly. I think it is similar to a expression ?: action construct, only it acts on a boolean false instead of a null.

Does this seem clear to you? If no, how would you refactor it?

Dennis Haarbrink
  • 111
  • It is an extremely common idiom, though using || for control flow like this is a bit iffy, so it's understandable that some people don't like it. The alternative is an if statement, though this requires the condition to be negated. Another alternative is to throw the exception from within the validator function, which tends to be safer and can sometimes lead to better error messages. – amon Mar 29 '24 at 08:33
  • @amon I considered both your alternatives. As you said, the if requires the condition to be negated, which I am not a fan of. And throwing from the validator requires a try/catch/throw from the consumer side. That's not an improvement to me either. – Dennis Haarbrink Mar 29 '24 at 08:43
  • To be honest, the thing that I wonder is why failed validations need to throw an exception at all. Can you [edit] you question to include more context, and why it is desirable to throw an exception? Furthermore, asking "does this seem clear" will attract opinionated answers. Can you define the problem with this code? – Greg Burghardt Mar 29 '24 at 11:57
  • @GregBurghardt In this specific case a 422 would have been returned in case of a failed validation. That can either be done by throwing a HttpException or returning a ResponseEntity, but in either case the validate || would have stayed the same. How can I rephrase my question to become fact based? Isn't this type of question opiniated by definition? – Dennis Haarbrink Mar 29 '24 at 13:33
  • 2
    I don't mind the || at all. What's bugging me is that the exception thrown isn't very helpful. – candied_orange Mar 29 '24 at 18:49
  • @candied_orange well, the example is simplified of course. The actual exception provides all the detail needed. – Dennis Haarbrink Mar 29 '24 at 19:36
  • @DennisHaarbrink that isn't a trivial concern to these kinds of examples. There are many alternatives to the if x throw new idiom that don't work well precisely because they new in weird places. That can foul up the stack trace and remove the original context. I actually don't see why you can't do a normal new on the same line as the ||. – candied_orange Mar 29 '24 at 20:46
  • @candied_orange this is Kotlin, we don’t use new at all. – Dennis Haarbrink Mar 30 '24 at 06:30

0 Answers0