Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question
11  
This is a rather poor Code Review question. If you gave us more contextual information (e.g. How did these variables get set? Is the message going to be used in a System.out.println() or embedded in a JSON HTTP response?) we could actually provide more useful advice than just shuffling your tokens around. – 200_success Feb 9 at 7:40
1  
It looks like you're doing validation, and I feel like at this point you have enough to justify factoring that out into its own method on your Trip class or whatever. – Blacklight Shining Feb 9 at 10:30
    
Hello, yes I'm doing validation. I managed to trim it down to 3 booleans by combining the destination checks (if the destination is blank, it's also invalid anyway). It's much more manageable now. :) – silver Feb 9 at 10:33
up vote 27 down vote accepted

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.

share|improve this answer
    
Hello, this looks cleaner than mine, thanks! – silver Feb 9 at 8:02
3  
As a matter of style, I do this as well, but the !isNameBlank gets a few extra spaces. I've seen some folks do String message = ( true \n && !isNameBlank \n && isDestinationValid... just so the && lines up, although myself I'm not a fan of going that far. – corsiKa Feb 9 at 21:48
1  
When I read this, I am confused because you assign a boolean value to a string variable. With the ternary operator being on the same indentation level as the variable declaration, I don't recognize it as part of the same statement. – Sebastian Redl Feb 11 at 10:27

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!";
share|improve this answer
    
Thank you, this is also a nice option. Upvoted. – silver Feb 9 at 8:02
2  
I much prefer this to the accepted answer, although I think SoC and SRP demands this be refactored into a method. get*Some*Message():String which returns a result depending on the result of calling isTripPossible():bool – kai Feb 9 at 10:43
    
Note that the binary operators should go at the beginning of the lines, not the end. At least this is what Knuth says (and he does so after extensive typesetting research and use). – Bakuriu Feb 10 at 21:19
    
source? my position on this is that a line should end with "whats next" meaning =, &&, ||, ; as to say "I am this", "AND next one" , "we are done here". – Zesar Feb 11 at 8:49

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?

share|improve this answer
1  
I'm all with you on this. as wrote, if the logicals are members then maybe you would like them to have accessors. My understanding was that the scope of the problem was "how to clean the code up within a method/function", ie. local vars in the method-context. – Zesar Feb 9 at 14:39
1  
Mine too, I'd use functions for that! We may need little bit more context from OP. – Adriano Repetti Feb 9 at 14:44
1  
@silver I see they're local variables, I'd still move them to their own function if they're even slightly more than trivial. doPost() method is doing many things and you need to read it all to understand what it is doing. That's usually a pretty bad thing, in ideal world to understand such high level feature I would read just 10~20 lines of code (less is better) because in two seconds I can get an overview of what should happen. Is it that method too long to post it here (you may get a better review)? Then it probably needs to be refactored... – Adriano Repetti Feb 10 at 7:48
1  
Technically there are a lot of solutions. You put the code on the semantic trial. Thats what I would do as well because simply make the code more compact is only sometimes the solution but not always. As we see only little code with some names we cannot judge it in the whole context it will be used. But as the names are very expressive we can get a vague idea whats the purpose. So combining things that belong together and separating things that do not are things on the semantic level that should drive your code. That was clearly pointed out here. – oopexpert Feb 10 at 12:18
1  
"Utility/helper classes are more often than not a big code smell." - This got me interested into reading about the matter here and here. It seems there's no unanimous take on this with several commenters even attributing it to "oo purism". Seeing the ProgrammersExchange thread was also closed for "possibly soliciting debate/arguments", it appears it's more of an opinionated discussion than a factual one. – silver Feb 10 at 17:08

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.

share|improve this answer

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() + ".";
}
share|improve this answer
2  
That's more useful- even better would be to summarise all the precondition failures, so the user doesn't get into a round of "what else to I have to change to satisfy this machine...?" My usual technique is to accumulate the errors into a vector; if the vector is empty then we can proceed, else join its elements into the error message. – Toby Speight Feb 10 at 10:20
1  
@TobySpeight I like the idea, the problem is that you have to start staggering your logic if you do that. Certain errors don't make sense like "Invalid destination" if you also have "Blank destination". I see your point though. – corsiKa Feb 10 at 15:53

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!

share|improve this answer
    
How will the user know what they need to change when they try again? – corsiKa Feb 10 at 5:16
    
Hello, this particular project is not meant to be that complex. It's only barebones for demo purposes. – silver Feb 10 at 15:32
1  
Is there a limitation to what needs to be reviewed? – silver Feb 10 at 15:55
1  
I didn't expect this simple question to garner that much attention to be honest (or that code review here is only for production code apparently). Should the admins/moderators see it fit, I don't mind them closing the thread. Thank you. – silver Feb 10 at 16:05
1  
@corsiKa could you reference your "reviewed as if they were production code" claim? It sounds ridiculous that code review should only be for "production code". There are many more interesting forms in which good code can reside. – Per Eriksson Feb 11 at 9:01

protected by Mat's Mug Feb 9 at 17:39

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.