javadesign-patternshibernate-validatorspring-validator

Which design pattern to use to avoid if/else in validation classes?


I am currently using HibernateConstraintValidator to implement my validations. But my reviewer is not fine with having if/else in code or ! operators. Which design pattern can I use to remove the if/else in my validation logic?

public class SomeValidatorX implements ConstraintValidator<SomeAnnotation, UUID> {

      @Autowired
      SomeRepository someRepository;
   
      @Override
      public boolean isValid(UUID uuid, ConstraintValidationContext context) {

             return !(uuid!=null && someRepository.existsById(uuid)); //The reviewer doesn't want this negation operator
      }
}

And in below code, he doesn't want if/else

public class SomeValidatorY implements ConstraintValidator<SomeAnnotation, SomeClass> {

      @Autowired
      SomeRepository someRepository;
   
      @Override
      public boolean isValid(SomeClass someObject, ConstraintValidationContext context) {
           if(someObject.getFieldA() != null) { //He doesn't want this if statement
                //do some operations
                List<Something> someList = someRepository.findByAAndB(someObject.getFieldA(),B);
                return !someList.isEmpty(); //He doesn't want this ! operator
           }
           return false; // He was not fine with else statement in here as well
      }
}

Side Note: We have to use Domain Driven Design (if it helps)


Solution

  • A long time ago, in the beginning of time. There was a guideline that said that methods should only have one exit point. To achieve that, developers had to track the local state and use if/else to be able to reach the end of the method.

    Today we know better. By exiting a method as early as possible it's much easier to keep the entire flow in our head while reading the code. Easier code means less mistakes. Less mistakes equals less bugs.

    In my opinion, that's why the reviewer doesn't like the code. It's not as easy to read as it could be.

    Let's take the first example:

     public boolean isValid(UUID uuid, ConstraintValidationContext context) {
    
             return !(uuid!=null && someRepository.existsById(uuid)); //The reviewer doesn't want this negation operator
      }
    

    What the code says is "not this: (uuid should not be empty and it must exist)". Is that easy to understand? I think not.

    The alternative: "Its OK if uuid do not exist, but if it do, the item may not exist".

    Or in code:

    if (uuid == null) return true;
    return !someRepository.existsById(uuid);
    

    Much easier to read, right? (I hope that I got the intention correct ;))

    Second example

          if(someObject.getFieldA() != null) { //He doesn't want this if statement
                //do some operations
                List<Something> someList = someRepository.findByAAndB(someObject.getFieldA(),B);
                return !someList.isEmpty(); //He doesn't want this ! operator
           }
           return false; // He was not fine with else statement in here as well
    

    Ok. Here you are saying:

    A easier way to conclude that is to simply say:

    Translated to code:

    if (someObject.getFieldA() == null) 
        return true;
    
    return !someRepository.findByAAndB(someObject.getFieldA(),B).isEmpty();
    

    In C# we have Any() which is opposite to isEmpty which I would prefer in this case as it removes the negation.

    Sometimes negations are required. It doesn't make sense to write a new method in the repository to avoid it. However, if findByAAndB is only used by this I would rename it to ensureCombination(a,b) so that it can return true for the valid case.

    Try to write code as you talk, it makes it much easier to create a mental picture of the code then. You aren't saying "Im not full, lets go to lunch", are you? ;)