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\$
  • \$\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\$ – 200_success Aug 21 '13 at 7:57
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\$
  • 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. Jul 29 '13 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 Jul 29 '13 at 19:49
  • \$\begingroup\$ Good idea, but the boolean logic is backwards. See my answer for an explanation. \$\endgroup\$ – 200_success Aug 20 '13 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\$
  • 1
    \$\begingroup\$ I can't change AddressCriteria because it's from an existing API. \$\endgroup\$ – JREN Jul 29 '13 at 14:18
  • 8
    \$\begingroup\$ It could be wrapped in another class though. \$\endgroup\$ – James P. Jul 29 '13 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\$
  • \$\begingroup\$ AddressCriteria is from an existing API, I can't change it. \$\endgroup\$ – JREN Jul 29 '13 at 14:17
  • \$\begingroup\$ This code : String s; s.isEmpty(); throw an Exception \$\endgroup\$ – cl-r Aug 20 '13 at 15:41
  • \$\begingroup\$ Sure, the variable s is not initialized. You would need String s = ""; \$\endgroup\$ – tallseth Aug 21 '13 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\$
  • \$\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\$ – 200_success Aug 20 '13 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 Aug 20 '13 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\$ – 200_success Aug 20 '13 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\$
  • \$\begingroup\$ What about reversing the logic (pseudocoding the functions for brevity): return !(nullOrEmpty(City) && nullOrEmpty(Number) && nullOrEmpty(State) && nullOrEmpty(ZipCode)); \$\endgroup\$ – Izkata Jul 29 '13 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 Jul 30 '13 at 6:22
  • \$\begingroup\$ I think Strings.isNullOrEmpty() is from Guava or an external library, you didn't mention it. \$\endgroup\$ – Marc-Andre Aug 20 '13 at 14:51
  • 1
    \$\begingroup\$ @Marc-Andre Thanks, I've fogotten that is a home made tool. \$\endgroup\$ – cl-r Aug 20 '13 at 15:12
  • \$\begingroup\$ You got your boolean logic backwards — your solution is not equivalent to the code in the question. \$\endgroup\$ – 200_success Aug 20 '13 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\$
  • \$\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 Jul 30 '13 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\$ – Ismail Badawi Jul 30 '13 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 Jul 30 '13 at 13:35
  • \$\begingroup\$ @IsmailBadawi Corrected, thanks. You can tell I'm not really a Java programmer. \$\endgroup\$ – zwol Jul 30 '13 at 13:35
  • \$\begingroup\$ Yes, Java uses reference semantics for everything but basic types (int, char and so on). \$\endgroup\$ – Lstor Jul 30 '13 at 13:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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