Back to the Code

After a lot of posts talking about Linux admin (why!?) I thought it was time to do a code review.

From today’s job interview. What’s wrong with the following function?

public boolean validate(Page p) {
    return (p== null || p.getText() == null || 
            p.getUrl() == null || p.getUrl().trim().equals("") || 
            p.getText().trim().equals(""))? false:true;
}

Seems like an innocuous bit of code, doesn’t it? Yet there are the following things wrong with it:

  1. Where’s the Javadoc? It clearly needs it because…
  2. What’s with the name – a boolean function called validate… does true mean it’s valid? Are we sure? Should it be “isValid” just to be sure?
  3. Why is the “page” called “p”? A bit brief there.
  4. What’s with the unnecessary brackets?
  5. Why is the validation of p.getText interrupted by the validation of p.getUrl only to be resumed?
  6. What’s with the repetition of the string validation? Should we create a function “isNullOrBlank” and reuse it for both strings?
  7. What’s with the use of .equals(“”) – did we not have String.isEmpty()?
  8. What’s with the ternary expression? Functions that return a boolean should just return it, not run it through a ternary…
  9. … and why does this function test for false only to have to negate it?
  10. And couldn’t this be static?
  11. And what’s the cyclomatic complexity of that one function!?

Could you write the above test without 11 TODOs on code review? If so, get in touch.

Here’s the code as you could have written it (assuming we really didn’t want it static for some reason):

/**
 * @return whether the page is valid
 **/
public boolean isValid(Page page) {
    return page!=null && 
           isValid(page.getText()) && isValid(page.getUrl());
}

private boolean isValid(String string) {
    return string!=null && !string.trim().isEmpty();
}

The above is not perfect, but I think I could live with it more. Especially if it were covered by unit tests that enabled further refactoring.

Advertisements

2 comments

  1. If multiple null check needed, lambda+optional can help. Example:

    public static Optional resolve(Supplier resolver) {
    try {
    T result = resolver.get();
    return Optional.ofNullable(result);
    }
    catch (NullPointerException e) {
    return Optional.empty();
    }
    }

    resolve(() -> userRepository.findOne(item.getApp().getUser().getId())).ifPresent(oldItem::setId);

  2. True. It would be interesting to see how you’d apply the lambda to the above example. Possibly passing an Optional into the validator and immediately returning invalid if it was null.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s