allow for world names that don't follow the default scheme

[benjajaja]

If there are worlds other than one of world, world_nether, world_the_end, then a hashmap that maps uuids to thir UUID.hashCode() is used. UUID.hashCode() may have collisions, but will hopefully suffice for this scenario.

should close #47

[CivcraftBot]

Can one of the admins verify this patch? Type 'ok to test' to test.

[benjajaja]

ok to test

[ttk2]

so in theory there could be world name collisions on servers with unreasonable numbers of worlds?

[benjajaja]

In theory, yes. If you're thinking sharding, there should only be one world per shard, if I understand bungee correctly.

[ttk2]

that's the plan so it should not be a problem for us.

[ttk2]

merging. if that's ok with everyone?

[ProgrammerDan]

@ttk2 a few fixes needed before merge.

[benjajaja]

@ProgrammerDan updated

[ttk2]

@ProgrammerDan @gipsy-king good now? If I put this online will it require other changes to my configs?

[benjajaja]

@ttk2 no it won't require any changes, but I haven't tested this one nor compiled it.

[ProgrammerDan]

should be good now @ttk2, I can test compilation tonight unless you just wanna do it live, heh.

Somewhat pedantic but @gipsy-king it would be best to include a "otherworlds != null &&" before each utilization, since that variable is not always initialized. I doubt it'll pop up too often but no point leaving NPE-honeypots if you can easily avoid them.

[benjajaja]

@ProgrammerDan wouldnt we then get the previous NPE anyway? Also this code won't be used on civcraft.

[ProgrammerDan]

Yeah I was figuring to add it so other servers don't run into NPEs -- as to your question, just the lookup functions. You can't know that someone will pass a valid value in -- something might go wrong when they are using the plugin, for instance, or perhaps they are getting clever and using RB as a dependency for a new plugin -- and if the first three if/else blocks fail to match, and you hit the otherworlds test, NPE unless you test for null.

[benjajaja]

@ProgrammerDan I don't know what to do. In my javascript world where only runtime exception exists, I'd throw a descriptive exception. But here, if I check for null, I still have to return something crappy which will throw the NPE as the guy from the issue was getting. If I throw an exception, I have to try/catch at every call point, and this is out of scope for this case IMO, because then I'd have to add a bunch of logic I don't want to test right now.

The log warning is crappy too for sure, but at least next time we can identify what's wrong. I could imagine strange things this isn't prepared for, like a plugin adding worlds dynamcally at runtime and such, and I don't want to deal with that at all.

Can we just go with it? :smiling_imp:

[ProgrammerDan]

Well, all this would be solved if folks wrote some decent javadoc to describe expected return values :P.

You've got the crux of it, ideally these functions should intentionally throw an exception if the input cannot be validated as there is no valid output if the input cannot be matched to a world. However, and it's a practical decision, you're going the C route of returning -1 if the UUID can't be found, and null if the "id" can't be found. So be consistent. Never introduce a state where instead of returning null, you throw NPE. If there exists a valid state where the function returns a value on invalid input, make that your standard return for invalid input unless whatever causes the NPE is completely beyond your ability to predict.

tl;dr Coding best practice in java: If you return a non-error value for some bad inputs, return it for all expected bad inputs, or stop returning null and always intentionally throw an Exception for bad inputs.

Either that or just document it in javadoc at the top of the method, like "If you have non-standard worlds, but you ask for one the method can't find, returns null. If you don't have non-standard worlds, but ask for a non-standard world, throws NPE."

(I'm so wordy, sorry) Just in general, prevent and control every exception scenario, NPE's are horrible, don't let them pass you by if you can prevent it. I've eaten enough of my own crow even just over the past few weeks for not capturing NPE states and preventing.

[benjajaja]

I am enlightened, push -f will follow shortly :)

[erocs]

Also this code won't be used on civcraft.

When sharding is enabled it will be.

Null null null

It seems that if otherWorlds is initialized statically (and make otherWorlds final) it would never be null under normal circumstances. The Integer value might be null, but returning null is already an error indicator for getMCID() so that's fine.

[benjajaja]

When sharding is enabled it will be.

It really shouldn't, one server should have no more than one world, not even nether nor the end, otherwise sharding is kinda pointless.

That of course is no excuse for ambiguos code, I'm following Dan's guidelines.

[ProgrammerDan]

I think he meant the names of the worlds would be non-standard, hence the code would be active.

[benjajaja]

Oh right. Let's hope no civplugins depend on "default" world names :sweat_smile:

[benjajaja]

@ProgrammerDan see if I got this right, also using entrySet.

[ProgrammerDan]

Yeah, looks better.