Conversation
tadhunt
changed the title
garbagemule
left a comment
garbagemule
left a comment
There was a problem hiding this comment.
As much as I'd like to silence the ArenaRegion warning, I don't like the idea of introducing a "no-op null check", because it could just make the code that more confusing ("why is there a null check here, when it doesn't seem to ever be the case?"). A complete refactoring of the region class is a pretty big ask, though, and definitely not an adventure I expect anyone to embark on 😛
I think without the ArenaRegion change, the WaveParser change would probably fit better in the warning cleanup PR along with the other default branch changes.
| if (lower != null) setLocation(coords, r1.name().toLowerCase(), lower); | ||
| if (upper != null) setLocation(coords, r2.name().toLowerCase(), upper); | ||
| if (lower != null && r1 != null) setLocation(coords, r1.name().toLowerCase(), lower); | ||
| if (upper != null && r2 != null) setLocation(coords, r2.name().toLowerCase(), upper); |
There was a problem hiding this comment.
It took me a while to figure out what the heck the code in this method actually does; it's a bit messy to say the least...
This change is not actually necessary, because in all the branches of the switch statement, both r1 and r2 are assigned values. Except for the default branch, of course, which also assigns null to both upper and lower, which means the "transitive null checks" are sufficient. Also, the default branch can't be reached in the current state of the code base, because the only values ever given as RegionPoint are the ones in the four branches.
I can't really tell if it's worth making this code change in order to silence a warning. The underlying issue is that the code is much too messy, I think, so perhaps it would be better with a small refactoring of the method or the class in its entirety?
| default: | ||
| throw new ConfigError("Unknown wave type for wave " + name + " of arena " + arena.configName() + ": " + t); |
There was a problem hiding this comment.
I'm not sure why there is a warning about a default branch here, since the current branches are exhaustive. I guess it can't hurt to have it in case a new wave type is introduced, though.
|
Regarding refactoring, I can't commit to that as I don't really understand this code and am worried about breaking it in some non-obvious way. Re: requests above, I didn't see anything directly actionable from your notes. Did you have something specific I should change? Maybe we just add a comment to the changes in this PR that notes this needs a major refactoring? |
For sure. Unless you have an irrational desire to refactor moldy spaghetti code that takes several passes just to kinda understand the overall idea of, this isn't something I'd realistically expect anyone to do, so no worries 😛
I think the most consistent thing to do here would be to close this PR, but move the change with the |
|
SGTM! |
2 participants
Summary
Problem
Fix code paths that might result in null indirections
Solution
Action