Most readable way to write simple conditional check

Question

What would be the most readable/best way to write a multiple conditional check such as shown below?

Two possibilities that I could think of (this is Java but the language really doesn't matter here):

Option 1:

   boolean c1 = passwordField.getPassword().length > 0;
   boolean c2 = !stationIDTextField.getText().trim().isEmpty();
   boolean c3 = !userNameTextField.getText().trim().isEmpty();

   if (c1 && c2 && c3) {
      okButton.setEnabled(true);
   }

Option 2:

   if (passwordField.getPassword().length > 0 &&
         !stationIDTextField.getText().trim().isEmpty() &&
         !userNameTextField.getText().trim().isEmpty() {
      okButton.setEnabled(true);
   }

What I don't like about option 2 is that the line wraps and then indentation becomes a pain. What I don't like about option 1 is that it creates variables for nothing and requires looking at two places.

So what do you think? Any other options?

Answer 1

I would modify option 1 so that you're using variable names that actually have a meaning. That is, change the name of "c2" to be something like "stationIDIsEmpty" (and move the NOT into the conditional). That way the conditional is readable without having to glance back and forth for every variable.

So my code would probably look like:

boolean enteredPassword = passwordField.getPassword().length > 0;
boolean stationIDIsEmpty = stationIDTextField.getText().trim().isEmpty();
boolean userNameIsEmpty = userNameTextField.getText().trim().isEmpty();

if (enteredPassword && !stationIDIsEmpty && !userNameIsEmpty) {
   okButton.setEnabled(true);
}

Answer 2

if (HasPassword() && HasStation() && HasUserName())
  okButton.setEnabled(true);


bool HasPassword() {
 return passwordField.getPassword().length > 0;
}

etc.

Answer 3

Personally, I like the second way, because I find that using that way can make the predication of the conditionals clear. That is, with that method done properly, you can make the conditional comprehensible by "verablizing" it (whether or not you actually speak it is irrelevant).

That is, with your second option, it becomes clear that your conditional translates roughly as this: "If the password length is greater than zero, AND the stationIDTextField (trimmed) is NOT empty, AND the usernameTextField (trimmed) is NOT empty, then..."

Answer 4

I prefer the following:

if (passwordField.getPassword().length > 0
    && ! stationIDTextField.getText().trim().isEmpty()
    && ! userNameTextField.getText().trim().isEmpty())
{
    okButton.setEnabled(true);
}

With this coding style I accomplish two things:

• I can easily see that each extra line of the if is part of the condition because of the && (or ||) at the beggining.
• I can easily see where the if statement ends because of the { at the next line.

Answer 5

Option1 is prime for applying the refactoring 'Replace temp with Query'. The reason being that someone can stuff in code between the variable is initialized and the check and change the behavior of the code. Or the check might be made with stale values.. an update has been made to the textfields between initialization and checking.

So my attempt at this would be

if (GetPasswordLength() > 0 
   && FieldHelper.IsNotEmpty(stationIDTextField) 
   && FieldHelper.IsNotEmpty(userNameTextField) 
{
   okButton.setEnabled(true);
}

FieldHelper is a class with public static methods (also called a Utility class / Static class in C#)

Answer 6

Note that option 1 does not allow for short circuiting behavior. That is, you calculate the value of all of the conditionals before evaluating the result of the first.

Answer 7

I voted for Chris Brandsma's answer.

But just wanted to mention the main issue I have with Option 1 is you are losing the benefit of &&. With option one, although I think it's more readable, you are processing comparisons when they may not be required.