CODEWITHSUNDEEP
java
Phoenix
Phoenix

Reputation: 8923

Any recommendation on how to reduce the number of 'if' statements in this code snippet?

public void method() {
    returnValue = optional.absent();
    try {
        GetDate dateResponse = client.call();
        if (dateResponse != null) {
            myDate = dateResponse.getDate();
            if (myDate != null) {
                returnValue = convertDateToAnotherDate() //actual function
                if (!returnValue.isPresent()) {
                    LOG.error("Missing required fields");
                }
            } else {
                LOG.error("No myDate");
            }
        } else {
            LOG.error("Service returned no dateResponse");
        }
    } catch (CustomException exception) {
        LOG.error("Custom service failed");
    }

    return retVal;
}

Upvotes: 0

Views: 1506

Answers (3)

Bohemian
Bohemian

Reputation: 424993

You need the if's, but you can rearrange to make it a lot clearer an logically organised, following principles if:

  • fail early
  • minimise nesting
  • don't declare variables until needed/minimise their scope

After applying the above tenets:

public void method() {
    try {
        GetDate dateResponse = client.call();
        if (dateResponse == null) {
            LOG.error("Service returned no dateResponse");
            return optional.absent();
        }
        myDate = dateResponse.getDate();
        if (myDate == null) {
            LOG.error("No myDate");
            return optional.absent();
        }
        returnValue = convertDateToAnotherDate() //actual function
        if (returnValue.isPresent())
            return returnValue;
        LOG.error("Missing required fields");
    } catch (CustomException exception) {
        LOG.error("Custom service failed");
    }
    return optional.absent();
}

Notice how now the tests are all positive tests (using == rather than !=, which our tiny brains can comprehend better). The indentation (nesting) is reduced, so readability is up. The returnValue variable is only needed in the middle of the code too, so no need to declare it early.

Upvotes: 3

Makoto
Makoto

Reputation: 106400

Given the way you're reacting to what could happen if those values returned null, perhaps it's a better idea to throw exceptions from the calling methods, and log the exception out. You can also change your dateResponse to an Optional<GetDate>, and behave appropriately when it's absent.

Consider:

public Optional<Date> method() {
    Optional<Date> returnValue = Optional.absent();
    Optional<GetDate> dateResponse = client.call();

    if (dateResponse.isPresent()) {
        try {
            Date myDate = dateResponse.getDate();
            returnValue = convertDateToAnotherDate(date); //actual function
        } catch (IllegalStateException e) {
            LOG.error(e);
        }

    } else {
        LOG.error("Service returned no dateResponse");
    }

    return returnValue;
}

  • I'm assuming that client will return an Optional<Date>, and if it's present, we'll behave normally.

  • I execute normal business logic, as if the threat of null isn't possible.

  • If an error occurs (which I consider to be an IllegalStateException), I expect the methods that I call to throw it.
  • I log the exception that occurs, and provide a meaningful message when the exception is constructed.

An example structure of getDate could read like this:

public Date getDate() {
    if(date == null) {
        throw new IllegalStateException("No date present");
    }
    return date;
 }

...and now we're down to one if.

I genuinely don't know what types your variables are (I mean, I really don't - I asked if this would compile and I have my doubts), so this is about as far as I can go with the suggestion.

Upvotes: 0

Vulcronos
Vulcronos

Reputation: 3456

You can combine them, as in:

if(dateResponse!=null && dateResponse.getDate() !=null)

Upvotes: 0

Related Questions