11
\$\begingroup\$

I have a method that checks whether an addressCriteria contains only certain fields or more. If it contains more fields, it should return false, otherwise true.

I'm uncertain what the best way to write my if statement is. As far as I can tell, I see 2 options:

  • Write them in 1 if statement
  • Write each check in a different if statement

Single if statement:

private boolean isShortAddress(AddressCriteria addressCriteria) {
  if((null != addressCriteria.getCity() && !addressCriteria.getCity().isEmpty()) ||
      (null != addressCriteria.getNumber() && !addressCriteria.getNumber().isEmpty()) ||
      (null != addressCriteria.getState() && !addressCriteria.getState().isEmpty()) ||
      (null != addressCriteria.getZipCode() && !addressCriteria.getZipCode().isEmpty())) {
    return false;
  }

  return true;
}

Multiple if statements:

private boolean isShortAddress(AddressCriteria addressCriteria) {
  if(null != addressCriteria.getCity() && !addressCriteria.getCity().isEmpty()) {
    return false;
  }

  if(null != addressCriteria.getNumber() && !addressCriteria.getNumber().isEmpty()) {
    return false;
  }

  if(null != addressCriteria.getState() && !addressCriteria.getState().isEmpty()) {
    return false;
  }

  if(null != addressCriteria.getZipCode() && !addressCriteria.getZipCode().isEmpty()) {
    return false;
  }

  return true;
}

Maybe there's a 3rd or 4th option that I am unaware of?

\$\endgroup\$
1
  • \$\begingroup\$ Wow, so many responses have got the boolean logic inverted. Are Code Review users copying each others' mistakes? Or is isShortAddress() poorly named, such that it gives programmers the wrong impression about what it's supposed to do? \$\endgroup\$ Commented Aug 21, 2013 at 7:57

8 Answers 8

35
\$\begingroup\$

I would be compelled to write a helper that accepts a variable number of address fields and and returns true if all of the address fields are not null and not empty.

private boolean isShortAddress(AddressCriteria addressCriteria) {

  return hasFields( addressCriteria.getCity(), 
                    addressCriteria.getNumber(),
                    addressCriteria.getState(),
                    addressCriteria.getZipCode() );
}

private boolean hasFields( addressField... args )
{
  for( addressField arg:args)
  {
    if( null == arg || arg.isEmpty() )
      return false;
  }
  return true;
}

I did not test this code, but you should catch the drift.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ +1. This seems like a good approach because the null and isEmpty() checks are being carried out on all fields. \$\endgroup\$
    – James P.
    Commented Jul 29, 2013 at 15:01
  • 1
    \$\begingroup\$ And use a name shorter than addressCriteria if all you're going to do is use it four times in a row! \$\endgroup\$
    – Rex Kerr
    Commented Jul 29, 2013 at 19:49
  • \$\begingroup\$ Good idea, but the boolean logic is backwards. See my answer for an explanation. \$\endgroup\$ Commented Aug 20, 2013 at 17:44
14
\$\begingroup\$

An AddressCriteria should know when its address is short so move this behavior into AddressCriteria is a good idea. Using @tomdemuyt code:

public class AddressCriteria {

    public boolean isShortAddress() {
        return hasFields(getCity(), getNumber(), getState(), getZipCode());
    }

    private boolean hasFields(final String... fields) {
        for (String field : fields) {
            if (Strings.isNullOrEmpty(field)) { // from google guava
                return false;
            }
        }
        return true;
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I can't change AddressCriteria because it's from an existing API. \$\endgroup\$
    – JREN
    Commented Jul 29, 2013 at 14:18
  • 8
    \$\begingroup\$ It could be wrapped in another class though. \$\endgroup\$
    – James P.
    Commented Jul 29, 2013 at 14:59
5
\$\begingroup\$

I think the second approach with multiple if statements is better because it is more readable.

Having multiple exit points allows to faster read source code as Martin Fowler explains in his book Refactoring: Improving the Design of Existing Code:

Nested conditional code often is written by programmers who are taught to have one exit point from a method. I've found that is a too simplistic rule. When I have no further interest in a method, I signal my lack of interest by getting out. Directing the reader to look at an empty else block only gets in the way of comprehension.

\$\endgroup\$
2
\$\begingroup\$

A reasonable alternative to @tomdemuyt's answer would be to also change the class so that those getters will never return null, but instead return empty string. I personally hate using null when a NullObject will suffice (like empty string).

I would move this method on to the AddressCriteria class, because it is a little better to ask an object about it's nature than to ask it a lot of details to decide it's nature with.

Finally, you could make the AddressCriteria class immutable, so that those properties can't change. Then they get set in the constructor, and you could make this check once and keep a boolean field for isShortFormAddress.

\$\endgroup\$
3
  • \$\begingroup\$ AddressCriteria is from an existing API, I can't change it. \$\endgroup\$
    – JREN
    Commented Jul 29, 2013 at 14:17
  • \$\begingroup\$ This code : String s; s.isEmpty(); throw an Exception \$\endgroup\$
    – cl-r
    Commented Aug 20, 2013 at 15:41
  • \$\begingroup\$ Sure, the variable s is not initialized. You would need String s = ""; \$\endgroup\$
    – tallseth
    Commented Aug 21, 2013 at 15:31
2
\$\begingroup\$

I think @tomdemuyt in on the right track, but unfortunately confused himself due to ambiguous method naming. Reading the logic in his hasFields(...) method, you'll see that it would be clearer to name it hasAllOfFields(...). That's not what was wanted in the original question.

Here is a corrected version of his solution:

private boolean isShortAddress(AddressCriteria addressCriteria) {
    return !hasAnyOfFields( addressCriteria.getCity(), 
                            addressCriteria.getNumber(),
                            addressCriteria.getState(),
                            addressCriteria.getZipCode() );
}

private boolean hasAnyOfFields(addressField... args) {
    for (addressField arg:args) {
        if (!(null == arg || arg.isEmpty()))
            return true;
    }
    return false;
}

An alternative way to express that would be:

private boolean isShortAddress(AddressCriteria addressCriteria) {
    return hasNoneOfFields( addressCriteria.getCity(), 
                            addressCriteria.getNumber(),
                            addressCriteria.getState(),
                            addressCriteria.getZipCode() );
}

private boolean hasNoneOfFields(addressField... args) {
    for (addressField arg:args) {
        if (!(null == arg || arg.isEmpty()))
            return false;
    }
    return true;
}

The second version reads better as English, but incorporating a negative sense into the helper method hurts its reusability, in my opinion.

\$\endgroup\$
3
  • \$\begingroup\$ @tomdemuyt should get most of the credit for the idea, and I considered just editing his post. However, I thought it would be important to leave the bug there for comparison as a cautionary tale to illustrate the danger of the ambiguous naming of hasFields(...). \$\endgroup\$ Commented Aug 20, 2013 at 17:43
  • \$\begingroup\$ I am fairly certain you are mistaken. A short address must have both city, number, state and zip. Your code returns true if any 1 of these fields is filled in. \$\endgroup\$
    – konijn
    Commented Aug 20, 2013 at 19:12
  • \$\begingroup\$ @tomdemuyt Read the original question again. If any of the fields is non-empty, it's supposed to return false. A short address must have none of those fields present. (I suppose that the presence of any one of those fields would make it a long address.) \$\endgroup\$ Commented Aug 20, 2013 at 20:05
1
\$\begingroup\$
. . .
if( //
    Strings.isNullOrEmpty(addressCriteria.getCity())  && //
    Strings.isNullOrEmpty(addressCriteria.getNumber())&& // 
    Strings.isNullOrEmpty(addressCriteria.getState()) && // 
    Strings.isNullOrEmpty(addressCriteria.getZipCode())) {
        return true ;
    }
return false;
....

It seems difficult to have more readable presentation.

EDIT after comment

    public static final boolean isNullOrEmpty(final String s1) {
        if (null == s1 || s1.isEmpty()) {
            return true;
        }
        return false ;
    }

or more simple as Zack wrote :

        public static final boolean isNullOrEmpty(final String s1) {
            return null == s1 || s1.isEmpty();
        }
\$\endgroup\$
6
  • \$\begingroup\$ What about reversing the logic (pseudocoding the functions for brevity): return !(nullOrEmpty(City) && nullOrEmpty(Number) && nullOrEmpty(State) && nullOrEmpty(ZipCode)); \$\endgroup\$
    – Izkata
    Commented Jul 29, 2013 at 17:57
  • \$\begingroup\$ Not certain it is a good idea : in my mind, at the first null found withe the '||' the method return false BUT it have to test all (or at least two) '&&' between the () to verify the logic of the test \$\endgroup\$
    – cl-r
    Commented Jul 30, 2013 at 6:22
  • \$\begingroup\$ I think Strings.isNullOrEmpty() is from Guava or an external library, you didn't mention it. \$\endgroup\$
    – Marc-Andre
    Commented Aug 20, 2013 at 14:51
  • 1
    \$\begingroup\$ @Marc-Andre Thanks, I've fogotten that is a home made tool. \$\endgroup\$
    – cl-r
    Commented Aug 20, 2013 at 15:12
  • \$\begingroup\$ You got your boolean logic backwards — your solution is not equivalent to the code in the question. \$\endgroup\$ Commented Aug 20, 2013 at 17:11
0
\$\begingroup\$

You might want to be a little bit more defensive and check not just empty strings but blank ones too. String.isEmpty() returns false for string with only whitespace characters.

Apache Commons Lang has a StringUtils.isBlank method for that.

\$\endgroup\$
-1
\$\begingroup\$

First, if (null != thing) can and should always be simplified to if (thing).
(Similarly, if (null == thing) can and should always be simplified to if (!thing).)
EDIT: Apparently you can't do that in Java, OK (bleah). You should still know that this is preferred style in several closely-related languages where you can do that.

Second, as several other people have pointed out, you have a repeated condition that should be factored into an isNullOrEmpty helper function.

Third, the construction

if (condition)
    return true;
return false;

is usually better written

return condition;

Putting it all together, we get

boolean isNullOrEmpty(String s)
{
    return s == null || s.isEmpty();
}

// ...

return (isNullOrEmpty(addressCriteria.getCity())   ||
        isNullOrEmpty(addressCriteria.getNumber()) || 
        isNullOrEmpty(addressCriteria.getState())  || 
        isNullOrEmpty(addressCriteria.getZipCode()));

Editorial aside: It is a bad code smell that the addressCriteria getters can return either null or an empty string. They should consistently pick one or the other to represent the absence of that field. However, you say that class can't be modified, so file that under "technical debt" and move on.

\$\endgroup\$
5
  • \$\begingroup\$ Java doesn't have an equivalent for most uses of const, including const*. However, final in Java means that a pointer can't be reseated, i.e. the same as x* const. \$\endgroup\$
    – Lstor
    Commented Jul 30, 2013 at 2:51
  • 1
    \$\begingroup\$ Java only allows boolean expressions in conditionals. You can't replace if (null != thing) with if (thing); the latter is a compile error. \$\endgroup\$ Commented Jul 30, 2013 at 4:35
  • \$\begingroup\$ @Lstor Is Java default pass-by-reference? In C++ foo(String s) would copy s, and in particular it would be impossible for s to be NULL. \$\endgroup\$
    – zwol
    Commented Jul 30, 2013 at 13:35
  • \$\begingroup\$ @IsmailBadawi Corrected, thanks. You can tell I'm not really a Java programmer. \$\endgroup\$
    – zwol
    Commented Jul 30, 2013 at 13:35
  • \$\begingroup\$ Yes, Java uses reference semantics for everything but basic types (int, char and so on). \$\endgroup\$
    – Lstor
    Commented Jul 30, 2013 at 13:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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