Check whether address criteria contain only certain fields or more

Question

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?

Answer 1

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.

Answer 2

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;
    }
}

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

. . .
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();
        }

Answer 7

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.

Answer 8

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.