remove unnecessary casts and tests for null, discovered using NetBeans #1285
Conversation
stephengold
changed the title
|
Removal of unnecessary casts has been added to this PR. If there's no comment in 72 hours, I'll consider this approved for integration. |
MeFisto94
left a comment
MeFisto94
left a comment
There was a problem hiding this comment.
I finally made it through all of 136 files :)
Thanks for that patch, Stephen.
I hope I didn't overlook some of the float calculations, but I guess the IDE doesn't make mistakes there, usually.
Looks fine, if my comments are addressed (most of them could be a separate PR, as they are suspicious code that I stumbled over while looking through that code snippets, but since this is a cleanup PR I wouldn't mind having them addressed here).
| } | ||
| } | ||
|
|
||
| if (vertexGroups != null && vertexGroups.size() > 0) { |
There was a problem hiding this comment.
The IDE suggested this, because an Exception would be thrown in Line 222.
That being said, can we verify if vertexGroups should be non null? If not, that if belongs in Line 222, much like it is in Line 219.
I suspect that only the Vertex Colors are optional, though, and thus the author just wanted to be sure.
There was a problem hiding this comment.
mmm... blender importer. Took me a sec to figure out what the heck was going on here but it all makes sense now.
There was a problem hiding this comment.
If vertexGroups can be null, then I think fixing the resulting NPE should be a separate PR, right?
| } | ||
| }); | ||
| ((SphereProbeArea) probe.getArea()).setRadius(100); | ||
| probe.getArea().setRadius(100); |
There was a problem hiding this comment.
I am not sure about this one.
IIRC getArea() is a deprecated helper and the future is to have multiple area types, so here we'd introduce a Deprecated Method in exchange for the correct approach.
There was a problem hiding this comment.
I mean, the removal of the cast or not doesn't change that. It was using the deprecated method before, too. I wonder what the alternative should be.
There was a problem hiding this comment.
We'd need to find nehon's comment on that, but basically something like if (probe.getArea() instanceof SphereProbeArea) etc.
Or here specifically keeping the cast, as getArea is not deprecated, but the assumption that it can only return a SphereProbeArea is
But we should double check that again with the reality.
There was a problem hiding this comment.
LightProbe.getArea() is not deprecated. Perhaps you were thinking of LightProbe.getBounds()?
There was a problem hiding this comment.
Oh yeah, I meant getBounds(), that was my source of confusion!
I wonder why we have a separate class "SphereProbeArea" though, for something that could be a float, when we don't plan to have multiple Area Types.
jme3-terrain/src/main/java/com/jme3/terrain/geomipmap/TerrainPatch.java
Outdated
Show resolved
Hide resolved
jme3-terrain/src/main/java/com/jme3/terrain/geomipmap/TerrainPatch.java
Outdated
Show resolved
Hide resolved
jme3-terrain/src/main/java/com/jme3/terrain/geomipmap/TerrainQuad.java
Outdated
Show resolved
Hide resolved
3 participants
remove unnecessary tests for null, discovered using NetBeans