Skip to content

Fix potential null indirections #774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tadhunt wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix potential null indirections #774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/com/garbagemule/MobArena/region/ArenaRegion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,8 +524,8 @@ private void setPoint(RegionPoint point, Location l) {
}

// Set the coords and save
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);
Comment on lines -527 to +528

@garbagemule garbagemule Oct 21, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

save();

// Reload regions and verify data
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/garbagemule/MobArena/waves/WaveParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ public static Wave parseWave(Arena arena, String name, ConfigurationSection conf
case BOSS:
result = parseBossWave(arena, name, config);
break;
default:
throw new ConfigError("Unknown wave type for wave " + name + " of arena " + arena.configName() + ": " + t);
Comment on lines +101 to +102

@garbagemule garbagemule Oct 21, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Grab the branch-specific nodes.
Expand Down