[Plugin/API] ServerControl

Do you plan on extending UserData with any other objects? As it stands, it seems a bit pointless to have UserData with only 2 members that can easily be merged into the User object.

Also, what are your ideas behind the ‘lastMSG’ variable in the User class? After seeing the methods for it, I suppose it’s for correspondence, so the object can retain who was replied to last, more or less.

You were correct with the second part.

The reason for the UserData class is because, well it basically serves the same purpose as the TimeManager class, eventually I’ll add more to both classes and ya know… simplification :smiley:

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 >.>

1 Like

[size=10]I just used § codes.[/size]

I was taking a look at your edits of my plugin and I have some notes:

Main class

  • You don’t need to instantiate the utils based on the code I had before your edits.
  • For the event manager, I have a class for this, my class may not be 100% necessary, but once I start building up more Listener classes, I feel it would be better to keep my class.
  • I forgot to include this, thank you for reminding me.
  • I also forgot about this, thank you again.
  • You removed the variables for the FileManager and Logger… which is necessary, as well as their getters which are necessary to access them from a separate class which I intend to do. I have a feeling that you don’t actually know that this plugin has a long way to go, which is probably impacting your edits.

User class

  • Still unsure why everyone wants me to use the User class to store my User class instances
  • The session id is NOT a UUID, it’s just the position a User appears on the user list, but I think having a UUID for a session ID could come in handy!

The other stuff I’m just looking through, thank you for the effort though. And I’ll be sure to give you credit where it’s due!

A lot of those links go to the same line in the same class, so I’m not sure what you mean in some places.

Most of your utils had non-static members, so I believe those need to be instantiated, generally.

The variable I removed from Logger was (sort of) accessible from the Main object (if i recall correctly), which was the name of the plugin, which was stored in two areas, and assumed that the logger’s name wouldn’t need to change. Don’t recall what I removed from the FileManager, but I think it may have been the static method that accessed a static reference to itself, and seemed really unnecessary since if it’s static, you just access it’s methods via the class name. Since all the methods in the FileManager were static, it seemed fine to remove references to itself. You do NOT need getters to fetch static objects in other clases! To use the methods in any of the Utility classes I modified, you simply use the static context, i.e. Logger.severe("Message");, there’s no need to do Main.getLogger().severe("Message");. Partly why I made all the of util classes static was so that I could eliminate unnecessary getters.

Have a static object in an object that’s instanced is perfectly fine, it just means that you need to use the class name to access, not an instance of a User, and it seems more intuitive that may (so I’ve come to think). It may make more sense later to have the cached User object list stored in a separate object if there’s more static data to store and manage, but when there was just a single array, then it seems more fit in the User class. In general, also allows you to import fewer objects.

I used a UUID for session IDs because it seemed like those should remain unique, and in some instances, collections won’t retain the order objects are added to them, so somewhat safer to use UUIDs, although in a List, it’s likely fine. If you need to know the index that an object is stored in a collection, there’s usually an indexOf(obj) method for the collection to fetch just that without having to store it somewhere else.

Edit: After writing this, I think a lot of confusion here is possibly if you’re misunderstanding what static implies when prepending data members of a class. I can try explaining it a bit more, but it can be a bit tricky to put into words.

Here’s a bit of my static explanation. Static’s technical definition I think is something like “only one per class”, which implies that any static data member (member including any method or variable) is not instanced. So in effect, if I have a class

public class CoolThing {

	private static List<CoolThing > cachedCoolThings 
			= new ArrayList<CoolThing >();

	private Object thing;

	public CoolThing (Object thing) {
		setThing(thing);
		cachedCoolThings.add(this);
	}

	public Object getThing() { return thing; }
	public Object setThing(Object thing) { this.thing = thing; }

	public static boolean isCoolThing(Object thing) {
		for (CoolThing ct : cachedCoolThings)
			if (ct.getThing().equals(thing))
				return true;
		return false;
	}
}

Note this vaguely resembles the structure I used in your User class, how this class caches instances of itself. It can do so because the cached list is static, meaning it won’t get re-initialized when a new CoolThing instance is created. To use these members, we can do something like this from another class (regardless of package):

Object thing = (Object)("A Thing!"); // Probably don't need to cast, whatevs
CoolThing ct = new CoolThing(thing);

// If we want to check if thing is a CoolThing, we can use the .isCoolThing(Object)
// method, which is static, so access it through 'CoolThing', not an instance of it
if (CoolThing.isCoolThing(thing)) { /* it is */ }

// However, if we want to get the internal 'thing' Object, then we need to use
// non-static members, so we access it through instances of CoolThing, rather
// than through the class itself
thing = ct.getThing();
// This also applies to setting it. Mutators and getters are not generally static
// for these sort of classes as they fetch and alter instances of a class
ct.setThing(thing);

Although I thinkt he IDE allows you to fetch Static objects in a non-static context in some instances, it’s generally a bad idea, although you can do so if you’re referring to another static member from a static member within the same class. i.e.:

public class UselessClass {
	private static boolean trueVariable = true;
	private static boolean getTrueVariable() { return trueVariable; }
	public static boolean isTrueVariableTrue() {
		return getTrueVariable();
		// Even though getTrueVariable() is static, you do not need to access
		// it through the class if it's within the same class and both members
		// are static. Notice that getTrueVariable() doesn't use the class
		// name to access trueVariable either, since the same rule applies
		// to variables in the same class
	}

	// In contrast to when you can ignore static access, consider this example
	public boolean isTrueVariableFlase() { // <-- Not static!
		return UselessClass.getTrueVariable()
		// In this instance, you still must use static context because we're
		// accessing a static member from a non-static class
	}
}

Hope this clarified some stuff about static class members. Will edit this a bit more than likely if I find something wrong with it. In retrospect, I realize my teacher sucked at teaching us about static members. Makes me think I may have a shot at introductory java teaching XP