Coverty triage, phase V

[chrisbennight]

Vast majority were just potential null pointer dereferences. With this we are down to 0.

[rwgdrummer]

Per our conversation. I am going to go through these and see if we can employ a different strategy in some cases, to avoid proliferation of null checks in the code.
Seems to be unanimous that

• meaningful exceptions are better in some cases (e.g. getScanner()),
• annotated ignores with good explanations where coverity mistaken (e.g. some of the get property stuff in analytics)
• chasing and fixing down root causes of null when it should not happen by analyzing coverity's analysis. The fix may be an exception.

[chrisbennight]

That's probably a better choice - I started down the exception path in some cases, but then backed it out. I figured I'd toss this up as a discussion starter :)

@rfecher @jwomeara @spohnan (Including the others) -
I would actually be fine if we want to toss out as a coding standard/convention that we just never return null period (obviously with external libraries we can't always control that, but whenever possible). For exception cases throw a meaningful exception, and let it be handled at the level it makes sense; otherwise things like returning an empty colleciton, etc. might be reasonable routes depending on the methods return type.

[jwomeara]

Are we talking strictly about null return values? Generally, I think null
is useful in certain situations. Perhaps not as a return value though.
On Mar 20, 2015 7:15 PM, "Chris Bennight" notifications@github.com wrote:

That's probably a better choice - I started down the exception path in
some cases, but then backed it out. I figured I'd toss this up as a
discussion starter :)

@rfecher https://github.com/rfecher @jwomeara
https://github.com/jwomeara @spohnan https://github.com/spohnan
(Including the others) -
I would actually be fine if we want to toss out as a coding
standard/convention that we just never return null period (obviously with
external libraries we can't always control that, but whenever possible).
For exception cases throw a meaningful exception, and let it be handled at
the level it makes sense; otherwise things like returning an empty
colleciton, etc. might be reasonable routes depending on the methods return
type.

—
Reply to this email directly or view it on GitHub
#277 (comment).

[chrisbennight]

Yeah, return values specifically.

[jwomeara]

I think null is OK sometimes.

http://programmers.stackexchange.com/questions/120355/is-it-better-to-return-null-or-empty-values-from-functions-methods-where-the-ret
On Mar 20, 2015 7:24 PM, "Chris Bennight" notifications@github.com wrote:

Yeah, return values specifically.

—
Reply to this email directly or view it on GitHub
#277 (comment).

[chrisbennight]

We just have to move to java 8 so we have Option!

So something like the getDatastore() method. Returns null now if it can't get a datstore. I think it probably makes more sense to throw a DatastoreNotFound, IOException, or something else, right? Do we always expect it to find a datastore (I would say yes for method).

Something that returns a collection, returning the equivalent of Collection.Empty is probably better as well.

But I guess in a case where we had something that was GetFeature(String fid), it might make sense to return null if no feature was found (unless we expected somehow a feature to always be found, then an exception is fine).

I guess in retrospect probably just echoing eric's first point, and expanding a little (tend to exceptions instead of null if it's handling an error).

@rwgdrummer
https://scan.coverity.com/projects/3371?tab=overview
Our quota came up again - I re-ran master in coverity, it takes ~30 minutes to populate the dashboard. The easy way to run another branch in coverity is

git checkout coverity_scan
git reset --hard BRANCH_THAT_WILL_OVERWRITE_COVERITY
vi .travis.yml  // comment out all lines but one in the build matrix, or we blow our quota
git add .travis.yml; git commit -m "";
git push -f origin coverity_scan  //I always type in the branch just to make sure I don't hit something else.