1
\$\begingroup\$

I have the following piece of code and I would like to see what different approaches would be to solve that in a more elegant way.

The thing is that I don't know whether is better to have the null checks or the try/catch block there or maybe the code below should not happen in the first place. Maybe one can give another solution so that I can get the thumb URL from a images list that is set on a people object without having to check for null on each getter call.

if (people.getImages() != null && people.getImages().size() > 0 && people.getImages().get(0).getUrl() != null) {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
}

Or I can have:

try {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
} catch (Exception e) {
    // handle the null pointer exception
}

EDIT

For more information, I am interested more in knowing if there any other ways to avoid calling getter methods one after another until I get my desired data from an object and if the above code is good enough anyway.

The code works ok in both cases but I don't like to have the multiple null checks in that if statement and also catching exceptions is not a good solution either.

The getImages() method returns a list of Image objects and getThumb() returns an Url object that I need in order to fetch an image over the internet.

\$\endgroup\$
7
  • 3
    \$\begingroup\$ An avoided exception is always better then a handled exception. \$\endgroup\$ – Heslacher Jul 18 '14 at 11:40
  • 3
    \$\begingroup\$ This question appears to be off-topic because it is about best practices, and isn't asking for code to be reviewed. \$\endgroup\$ – Mathieu Guindon Jul 18 '14 at 11:46
  • \$\begingroup\$ This question appears to be very borderline as there are not much code here, there is however something that can be said about the code that is seen here. \$\endgroup\$ – Simon Forsberg Jul 18 '14 at 11:47
  • 2
    \$\begingroup\$ you haven't written the code yet @Cata, that is the problem. do you know that your code works? what is the context of the code? what is people.getImages? what is getThumb? we can't review this code, there isn't enough information. \$\endgroup\$ – Malachi Jul 18 '14 at 15:18
  • 1
    \$\begingroup\$ "or maybe the code below shouldn't happen in the first place." - probably yes, but we can't say concerning that, as you have given only that code, isolated from the context. This makes an actual review of the code at hand in the full context (useful / sane or not) impossible. Your edit does not actually make the purpose of the code any clearer. Voting to leave closed. More info in this meta-post esp: "The excerpt is hypothetical in nature" \$\endgroup\$ – Vogel612 Jul 21 '14 at 8:07
10
\$\begingroup\$

Let's count the number of times you're writing get.

if (people.getImages() != null && people.getImages().size() > 0 && people.getImages().get(0).getUrl() != null) {
    Url thumbUrl = people.getImages().get(0).getUrl().getThumb();
    // do something with the thumbUrl
}

5 times inside the if-condition and 4 times inside the block. That's a bit too much. Remember the Law of Demeter.

In your case I would do:

if (people.hasImage(0)) {
     Url thumbUrl = people.getImage(0);
}

The idea is to hide some logic inside the people object.

It also seems like people.getImages() is a Collection, which makes me think that it is a bad idea to ever let that be null. An empty collection is better than a null collection.

As for your main question, I believe Heslacher says it all:

An avoided exception is always better then a handled exception

This applies especially to NullPointerException and ArrayIndexOutOfBoundsException (and all other RuntimeExceptions).

Also, you should always catch the most-specific Exception possible. Never catch Exception. But then again, never catch NullPointerException. Fix your code to not cause one in the first place.

Related previous Code Review answers about exceptions:

Rethrowing exception just as an information

https://codereview.stackexchange.com/questions/45637/exception-handling-with-instanceof-rather-than-catch/45639#45639

\$\endgroup\$
0

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