How to simplify the code if there are many logical operators "or - ||"

Question

How to simplify the code if there are many logical operators "||"

    if (Boolean.TRUE.equals(fileBeforeCompressTmp1Passport > 10485760
            || fileAfterCompressionTmp1Passport < 10240
            || fileBeforeCompressTmp1Passport < fileAfterCompressionTmp1Passport

            || fileBeforeCompressTmp1Medical > 10485760
            || fileAfterCompressionTmp1Medical < 10240
            || fileBeforeCompressTmp1Medical < fileAfterCompressionTmp1Medical

            || value.phone().toString().length() != 10
            || validationRegExp.onlyNumbersRegExp(value.phone().toString())
            || serviceJpa.existsLogisticsPersonByPhone(value.phone())

            || value.email().length() < 8
            || value.email().length() > 58
            || validationRegExp.emailValidationRegExp(value.email())

            || value.password().length() < 6
            || value.password().length() > 24
            || validationRegExp.passwordValidationRegExp(value.password())

            || value.surname().length() < 1
            || value.surname().length() > 45
            || validationRegExp.onlyLettersCyrillic(value.surname())

            || value.name().length() < 1
            || value.name().length() > 45
            || validationRegExp.onlyLettersCyrillic(value.name())

            || value.middleName().length() < 1
            || value.middleName().length() > 45
            || validationRegExp.onlyLettersCyrillic(value.middleName())

            || value.dateBirth().toString().length() != 10
            || validationRegExp.validationDateRegExp(formattedString)

            || value.numberPassport().toString().length() != 10
            || validationRegExp.onlyNumbersRegExp(value.numberPassport().toString())
            || serviceJpa.existsLogisticsPersonByNumberPassport(value.numberPassport())

            || value.region().length() != 7
            || validationRegExp.onlyLettersCyrillic(value.region())

            || value.city().length() < 2
            || value.city().length() > 25
            || validationRegExp.onlyLettersCyrillic(value.city())

    ) {

        Files.deleteIfExists(ofPassport);
        return ResponseEntity.ok(new MessageResponse(HttpStatus.OK.value(), STATIC_OK));

    }

Below I show two methods, they are "emailValidationRegExp" and "passwordValidationRegExp". These methods validate the email address and password using a regular expression. "Bohemian" asked me to show them, so I show these methods.

        private static final Pattern patternEmail = Pattern.compile("^[\\w.-]*@[\\w-]*+.+\\w$");

        private static final Pattern patternPassword = Pattern.compile("^[0-9a-zA-Z@#$]+$");



        public boolean emailValidationRegExp(String email) {

            Matcher matcherEmail = patternEmail.matcher(email);

            return !matcherEmail.matches();
        }


        public boolean passwordValidationRegExp(String password) {

            Matcher matcherPassword = patternPassword.matcher(password);

            return !matcherPassword.matches();
        }

Answer 1

If possible I think you should dlegate the validation of the value object to its own class. I'd move the validations inside the class of value, I assume it is Passport, so something like this

class Passport {

    private String phone;
    private String email;
    private String password;
    private String surname;
    private String name;
    private String middleName;
    private String dateBirth;
    private String numberPassport;
    private String region;
    private String city;

    public boolean isPhoneValid() {
        return phone.toString().length() != 10
                || validationRegExp.onlyNumbersRegExp(phone.toString())
                || serviceJpa.existsLogisticsPersonByPhone(phone);
    }

    public boolean isEmailValid() {
        // ....
    }

    // .... Other isValidXxxx()


    public boolean isValid() {
        return isPhoneValid()
                || isEmailValid()
                || isPasswordValid()
                || isSurnameValid()
                || isNameValid()
                || isMiddleNameValid()
                || isDateBirthValid()
                || isNumberPassportValid()
                || isRegionValid()
                || isCityValid();
    }

}

Then have the compression validation in your class, in a separate method, and do

public static void main(String[] args) {

    if (validateCompression() || value.isValid()) {
        Files.deleteIfExists(ofPassport);
        return ResponseEntity.ok(new MessageResponse(HttpStatus.OK.value(), STATIC_OK));
    }


}

private static boolean validateCompression(
        int fileBeforeCompressTmp1Passport,
        int fileAfterCompressionTmp1Passport,
        int fileBeforeCompressTmp1Medical,
        int fileAfterCompressionTmp1Medical) {
    return fileBeforeCompressTmp1Passport > 10485760
            || fileAfterCompressionTmp1Passport < 10240
            || fileBeforeCompressTmp1Passport < fileAfterCompressionTmp1Passport
            || fileBeforeCompressTmp1Medical > 10485760
            || fileAfterCompressionTmp1Medical < 10240
            || fileBeforeCompressTmp1Medical < fileAfterCompressionTmp1Medical;
}

Answer 2

A solution to validate data using the validation framework

Since we do not have the entire code here, I'll assume you are actually performing various validations before taking an action on data. I assume that the data to validate comes from a remote client. But it could also work between two services I think. Perhaps it will help.

I see that you use ResponseEntity.ok(), so I could assume you work with SpringFramework.

The solution I propose will actually return an error (that you can turn into HTTP Status) if the conditions are not met, and will let program continue if the conditions are met (HTTP 200 / Ok).

If you have all the variables stored inside object fields, I'll call that a storage class here, you could possibly use the Java validation framework.

Once you have the validation framework in your dependencies (eg. spring-boot-starter-validation, for SpringBoot), you annotate the storage object parameter with @Valid annotation, to tell SpringFramework you want to run a validation here. It will then look for special validation annotations inside your object.

Inside your storage class (MyPayload), you can add annotations defining validation rules for each field. For example, min value, max value, Regular Expression pattern,etc.

//Method requesting the validation of one argument
ResponseEntity<MessageResponse> doSomeAction(@Valid @RequestBody MyPayload payload) {
        ...
        return ResponseEntity.ok(new MessageResponse(HttpStatus.OK.value(), STATIC_OK));
    }

//Storage structure to validate
@Getter // LOMBOK annotations, to generate getters and setters
@Setter
public class MyPayload 
{
    @Size(min = 3, max = 15)
    private String email;

    @Min(10485760)
    private long fileBeforeCompressTmp1Passport;
}

As you can see, the code will be more readable and you'll get rid of the big boolean comparison.

If the conditions are not met, an exception will be thrown. You can possibly handle the exception with an ExceptionHandler.

See more info here: https://www.baeldung.com/spring-boot-bean-validation

For anything that does not fit in that Validation Framework, you can still perform validation with dedicated methods, as suggested in other answers.

Seeing the fact I do not have the code around, I imagine this solution may not fit exactly your needs, but perhaps this will give you food for thoughts and you could a variation based on this. You could for instance have a method calling a Service validation method with a try/catch. If the solution logic does not work with ||, you can think it as a &&-logic and invert the Min/Max for instance.

Extra note: If you can't avoid copying a structure to another to fit this solution in your code, have a look at the library Mapstruct.

Answer 3

In general, you just have to split this checks with groupt and move each of it to the separate mehtod with apropriate name. It simplify your code.

Then you can create a separate class like ValueValidator and use it to encapsulate all checks there.

public class ValueValidator {

    private final ValidationRegExp validationRegExp = new ValidationRegExp();
    private final ServiceJpa serviceJpa = new ServiceJpa();
    private final String formattedString = "";

    private final Predicate<Value> isPhoneValid = value -> {
        Phone phone = value.phone();
        String str = phone.toString();
        return str.length() == 10 && validationRegExp.onlyNumbersRegExp(str)
                && !serviceJpa.existsLogisticsPersonByPhone(phone);
    };

    private final Predicate<Value> isEmailValid = value -> isInRange(value.email(), 8, 58)
            && validationRegExp.emailValidationRegExp(value.email());

    private final Predicate<Value> isPasswordValid = value -> isInRange(value.password(), 6, 24)
            && validationRegExp.passwordValidationRegExp(value.password());

    private final Predicate<Value> isDateBirthValid = value -> {
        Date dateBirth = value.dateBirth();
        return dateBirth.toString().length() == 10
                && validationRegExp.validationDateRegExp(formattedString);
    };

    private final Predicate<Value> isNumberPasswordValid = value -> {
        NumberPassword numberPassword = value.numberPassport();
        String str = numberPassword.toString();
        return str.length() == 10
                && validationRegExp.onlyNumbersRegExp(str)
                && !serviceJpa.existsLogisticsPersonByNumberPassport(numberPassword);
    };

    private final Predicate<Value> isRegionValid = value -> value.region().length() == 7
            && validationRegExp.onlyLettersCyrillic(value.region());

    private final Predicate<Value> isSurnameValid = value -> isValid(value.surname(), 1, 45);
    private final Predicate<Value> isNameValid = value -> isValid(value.name(), 1, 45);
    private final Predicate<Value> isMiddleNameValid = value -> isValid(value.middleName(), 1, 45);
    private final Predicate<Value> isCityValid = value -> isValid(value.city(), 2, 25);

    private final Predicate<Value> isValueValid = isPhoneValid.and(isEmailValid)
                                                              .and(isPasswordValid)
                                                              .and(isSurnameValid)
                                                              .and(isNameValid)
                                                              .and(isMiddleNameValid)
                                                              .and(isDateBirthValid)
                                                              .and(isNumberPasswordValid)
                                                              .and(isRegionValid)
                                                              .and(isCityValid);

    public boolean isValueValid(Value value) {
        return isValueValid.test(value);
    }

    public boolean isRangeValid(int before, int after) {
        return before <= 10485760 && after >= 10240 && before >= after;
    }

    private boolean isValid(String name, int lo, int hi) {
        return isInRange(name, lo, hi) && validationRegExp.onlyLettersCyrillic(name);
    }

    private static boolean isInRange(String str, int lo, int hi) {
        return str.length() >= lo && str.length() <= hi;
    }
}

In this case your code could be reduced to:

Value value = new Value();
ValueValidator valueValidator = new ValueValidator();

if (!valueValidator.isValueValid(value)
        || !valueValidator.isRangeValid(fileBeforeCompressTmp1Passport, fileAfterCompressionTmp1Passport)
        || !valueValidator.isRangeValid(fileBeforeCompressTmp1Medical, fileAfterCompressionTmp1Medical)) {
    Files.deleteIfExists(ofPassport);
    return ResponseEntity.ok(new MessageResponse(HttpStatus.OK.value(), STATIC_OK));
}

Answer 4

You could divide these into separate methods and have them return a boolean. Say you have a method like:

public boolean checkFileBeforeCompressTmp1Medical()
{ 
  //perform check
}

Then you can have an overarching method that runs all these check methods and assigns their results to a variable and check the result of the variable after the checks have ran:

public void runChecks()
{
    boolean checkResult = False;

    checkResult = checkFileBeforeCompressTmp1Medical();

    if (checkResult)
    {
        //do stuff
    }
}

Answer 5

Best practice would likely be to break your '||/or' statements into their own boolean sub-methods, grouped by related checks. I.E. grouping all the email checks in a method.

boolean methodName() 

{

if (email.length() < 8){
  return true
} //do this for each if, return false at the bottom.

}

If one of them returns true, the method returns true. Do this for each field, and then you can '||' them together in the main call and it'll look a bit cleaner. Depending on what you're trying to accomplish with your code, this may not be the most efficient, but it'll look cleaner.