I’m not sure XD It seems more intuitive to have it part of the User object, or to merge it all into the UserData object. Having them separate only seems to make sense if it’d be like EntityData and other non-user entities extended it later. Also, I have a few tips from what I’ve seen in the code so far.
I’ve seen several manual array iterations that aren’t necessary, particularly in the User class so far. Where you put something like
for (int i = 0; i < array.length; i++) { Object o = array[i]; }
and instead it can be
for (Object o : array) { /* o is already initialized */}
array being any object that is Iterable. In the case of HashMaps or Maps, you can use the keySet()
or values()
to get one half of the map or the other as an Iterable collection.
This applies to most instances of iterating over arrays unless you need the index position for some reason in processing the block. I’ve seen one block of code that used the index for something other than initializing, and mentioning that next:
In the deleteUser() of the User class, it looks like you attempted to remove an element of a collection whilst iterating over it, which won’t work (will throw an exception). Instead, try to use the .remove(Object o) method when possible.
In any instance where you’re comparing objects via ==
, make sure that you’re only comparing primitive data types or knowingly checking if two objects are exactly the same instance of something. Otherwise, use obj.equals(anotherObj)
, which is designed to compare two objects’ contents. Otherwise you may have some strange issues with two objects not being equal when they in fact have the same contents and data internally. It’s also good practice to @Override
the public boolean equals(Object obj)
method of your custom objects and compare internal data that should be equal in order for that method to return true when compare to other objects. I’ve done this in the version I’m working on for your User object, but just compares the internal Player object.
It’s generally also good practice to override the public int hashCode()
, but I don’t know how to do that without the Apache Commons HashCodeBuilder, although I’m sure there’s a way. (Bukkit had it built into it’s API, so was a bit simpler), just going to cheat for the User class and return the Player object’s hashCode() since the equals() only compares the Player object.
On to another tip! In instances where this sort of logic is applicable:
public boolean getSomething() { // Condensed to conserve space
if (x == true) return true;
else return false;
// Can instead be
return x;
}
And can be simply negated if your logic is backwards, i.e.:
public boolean getSomething() { // Condensed to conserve space
if (x != true) return true;
else return false;
// OR
if (x == true) return false;
else return true;
// Can both be put simply
return !x;
}
I’ve only seen one occurrence of this so far, so I suspect you know about that, just overlooked it.
I’ll try to send a PR when I’ve went through some stuff to show ya how I’d set it up, but fell free to decline it of course XP There’s a few things I’ve altered for naming conventions and to simplify things you might appreciate. I’ll fix errors in this as I see them I think >.>