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

Question

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

There's really not much to this question, is there...just a smattering of code...and a few magic variables, too...and your log variable is inconsistent. Does this truncated example even *compile* on your end? — Makoto, Sep 26 '13 at 01:09
Consider making `client.call` and `GetDate.getDate` throw an Exception on failure (if applicable)? I try to *avoid* null values if I can. — user2246674, Sep 26 '13 at 01:11
I think the way you're doing it is superior to most of the potential "improvements". Sometimes `if` statements just gotta be nested. — Hot Licks, Sep 26 '13 at 01:14
@Lesleh That is incorrect. `?.` is not available in Java 7 - http://stackoverflow.com/questions/4390141/java-operator-for-checking-null-what-is-it-not-ternary — Jeff Storey, Sep 26 '13 at 01:30
I'm tired and for a second I thought that Java 7 added the Elvis operator and somehow I never found out about it... Damn you, Lesleh, for getting my hopes up. — Chris Martin, Sep 26 '13 at 04:34

Bohemian · Accepted Answer · 2013-09-26T05:27:22.703

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.

score 0 · Answer 2 · answered Sep 26 '13 at 01:08

0

You can combine them, as in:

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

answered Sep 26 '13 at 01:08

Vulcronos

3,428
3
16
24

You would still need one more `if` for the case where `dateResponse==null && dateResponse.getDate() !=null` (or vice versa depending how you want to order the code). – David says Reinstate Monica Sep 26 '13 at 01:10
5

How in the world would dateResponse.getDate() be not null if dateResponse was null? You would just get a null reference exception. – Vulcronos Sep 26 '13 at 01:12
and this is java so it would short circuit anyway – Kevin Sep 26 '13 at 01:22
@Vulcronos You are totally right, I didnt read the code fully. – David says Reinstate Monica Sep 26 '13 at 02:07
@Dgrin91 It happends to all of us. – Vulcronos Sep 26 '13 at 02:09

score 0 · Answer 3 · answered Sep 26 '13 at 01:23

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.

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

3 Answers3