"Have a safe trip"… or not, depending on four conditions

Question

I have 4 booleans and all must be met to determine the value of a String:

String message = (!isNameBlank && !isDestinationBlank
    && isDestinationValid && isAmountValid)
    ? "Have a safe trip!" : "Please try again!";

If I do this, it doesn't look elegant either:

String message = "";
if(!isNameBlank && !isDestinationBlank
    && isDestinationValid && isAmountValid) {
    message = "Have a safe trip!";
} else {
    message = "Please try again!";
}

Is there a better convention when comparing many booleans in a statement?

Answer 1

To my knowledge there isn't a standard for this, but since it's a formatting question I'll address my preference for writing what you have and reasoning behind it.

String message = ( 
       !isNameBlank
    && !isDestinationBlank
    && isDestinationValid
    && isAmountValid)
? "Have a safe trip!" : "Please try again!";

1. variable declaration includes open parentheses and indicates something follows and is reminiscent of similar style for code blocks.
2. Each clause is on its own line which makes them both easily readable and modifiable regardless of statement length.
3. The && and || operators are at the beginning of the line, this is conventional and preferable when clause names are long, or nested
4. Ternary operator is lined up to initial declaration indicative of its end, and precedes the typical two returns on the same line.

Without more information I can't be too specific, but from the comments below your question, it seems like this would ideally be fully encapsulated into its own Trip class. In which validate is a method, or the constructor itself handles validation before instantiating the object.

Answer 2

When it comes to long and complex logic summation I recommend, as Legato says, to use one row for each condition. I also think it is more readable if you sum all the conditions to a new variable that explains the result with a meaningful name like this:

boolean isTripPossible = 
   !isNameBlank && 
   !isDestinationBlank && 
   isDestinationValid && 
   isAmountValid;      

String message = isTripPossible ? "Have a safe trip!" : "Please try again!";

Answer 3

Maybe late answer but I'd go to a completely different direction.

I think isDestinationValid variable (for example) doesn't materialize magically. There is an expression to calculate it and it's inside the function you use to determine which message you should return to your users: you're mixing logic and presentation in the same function. From comments I see you have all these logic inside your doPost() method. In my opinion it's too much, to understand what happen when you post data you have to go through code that:

• Fetches data from database.
• Validates your inputs both by their own and using DB data.
• Prepares output messages.
• Performs required task.

Waaaay to many things to do in one single method. When you have a complex logic condition then it probably deserves its own name (because it may be reused or because details may change in future). First of all I'd refactor out local variables introducing separate functions:

boolean canBookTrip() {
    return isExistingDestination() && isAccountInformationValid();
}

boolean isAccountInformationValid() {
    return isBillingNameFilled() && isValidAmount();
}

And so on. Of course take this with cum grano salis. When an expression is trivial and you won't reuse it then you don't need to have a function for that. For example isBillingNameFilled() may be a local variable however isValidAmount() and isExistingDestination() deal with database and I strongly suggest to make a separate function for them.

Few more notes:

• isExistingDestination() should both check for a valid (non-empty) and existing destination. It's not astonishing if an empty destination does not exist.
• It's absolutely not mandatory but usually not-negated logical conditions are easier to read. I'd make them all positive.
• Compiler will do optimizations for you then I'd split such long condition into multiple functions/local variables.

Your final code will then be:

String getTripBookingMessage() {
    if (canBookTrip())
        return "Have a safe trip!";

    return "Please try again!";
}

Note that your code stays in its own separate function. canBookTrip() will be probably reused somewhere else (book confirmation after submitting?) but you won't repeat same code again and again. Also, any change (for example, if you will allow a free pre-booking) won't affect all your code, but just this one very specific function.

EDIT for your own answer: you introduced InputUtils class. Utility/helper classes are more often than not a big code smell. Is there any reason they can't be private methods inside calling class? They will also should have access to HttpServletResponse. More than that you may want to have a Booking class responsible for booking (note that somewhere you should also manage concurrency) with booking information from Trip (using a Destination class). Also note that if (let's say) amountIsValid is false then you don't need to calculate amount with a dummy value. Also - let me repeat - I wouldn't do everything inside doPost() method. It's really no-no.

Note from comments: "...more often than not..." obviously does not mean always. Sometimes they're a viable and even good solution. However sometimes doesn't allow you to use them always. Not only because of academic OOP style reason but because helper methods are usually static and static methods are a nightmare to test. It doesn't matter if you adhere to TDD/BDD or not, for sure you need tests.

Ideally it should be something like this:

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    if (inputsAreValid(request))
        bookTrip(request, response);
    else
        rejectRequest(request, response);
}

This is important just for one reason: if you have to change something about booking then you directly go to bookTrip() and you can understand where to go in two seconds, looking doPost(). Moreover you're reasonably sure that anything you will do in bookTrip() won't affect rejectRequest(). If you have all your code in one function then you have to re-read everything every time you change something. For example, in your code, will changing destination also affects workflow when request is rejected?

Answer 4

Also a little late, but my preference is not to use the ternary for complex logic such as this. If I was writing this I would have written it like this:

String message = "Please try again!"; //defaults invalid

if(nameExists && destinationExists && destinationValid && amountValid) {
    message = "Have a safe trip!";
}

I made some other changes to suite my personal taste and make it easier (for me) to read.

Personally I prefer one line. It just makes it easier for me to read by scanning the line instead of breaking for the next line. If you can't make it fit on one line you might be doing to much and should break it out anyway. For example:

Bool destinationPossible = destinationExists && destinationValid;

I also made them all positive tests (would change the logic on the actual variable). Again just because it makes it easier for me to quickly read. (Similar to what Adriano recommended)

In this case I also dropped the "is" because it just didn't read right to me. To me isDestinationValid here implies a function call to determine the state where destinationValid is the current state of destination but this is just my preference. I think it comes from considering the function a question I am asking and considering the bool an assertion I have made.

Bool destinationValid = isDestinationValid()

If you really wanted to use the ternary I would also prefer you did the logic separately like @zesar's answer.

I also generally agree with Adriano's answer, however for this specific example in this context this is how I would have written it.

Answer 5

Personally, I would tell the user why they can't take the trip. I believe the method is pretty self describing. It's not perfect, but it gives the idea of how I would change the method.

public String getValidationMessage() { 
    if(this.getName() == null || this.getName().isEmpty()) {
        return "Cannot process trip without a passenger name.";
    }
    if(this.getDestination() == null || this.getName().isEmpty()) {
        return "Cannot process trip without a destination.";
    }
    if(!DestinationValidator.validate(this.getDestiniation()) {
        return "Cannot process trip to invalid destination " + this.getDestination() + ".";
    }
    if(this.getAvaliableFunds() >= this.getDestiniation().getCost()) {
        return "Cannot process trip to " + this.getDestination() + " costing + " this.getDestination().getCost() + " with available funds " + this.getAvailableFunds() + ".";
    }
    return "Enjoy your trip to " + this.getDestination() + ".";
}

Answer 6

This is just to show what I ended up with. Basically a slight mix of everyone's inputs:

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    boolean isNameFilled = InputUtils.isFilled(request.getParameter("name"));
    String name = isNameFilled ? request.getParameter("name") : "INVALID";

    boolean isDestinationValid = InputUtils.isDestinationValid(request.getParameter("destination"));
    String destination = isDestinationValid ? request.getParameter("destination") : "INVALID";

    boolean isAmountValid = InputUtils.isAmountValid(request.getParameter("amount"));
    Double amount = isAmountValid ? Double.valueOf(request.getParameter("amount")) : 0.0;

    String message = InputUtils.canBookTrip(isNameFilled, isDestinationValid, isAmountValid);

    // more code
}



public static String canBookTrip(
    boolean isNameFilled,
    boolean isDestinationValid,
    boolean isAmountValid)
{
    if(isNameFilled && isDestinationValid && isAmountValid) {
        return "Have a safe trip!";
    }

    return "Please try again!";
}

Thank you people!