Changes and Fixes

[ShawnClake]

An additional list of changes and fixes:

• In the overridden classes in RRPGEssentails, instead of returning new ArrayList<SomeType>() just return new ArrayList<>() and let the compiler infer the type.
• In AfkStatus, you made your class property public static. This means there will only ever be one instance of that class property. This means, if any player marks themself as AFK, all players are AFK, and if someone marks themself as present, everyone will be marked as present. ALL class properties should be marked as private. The only time it is desirable to mark something as public static is when only once instance if required. Even then, an access modifier of private static is more desirable.
• Remove the public static access modifier for all class properties except for the ones found in RRPGEssentials.
• When using class properties, which should almost always be marked as private, ensure you create getters and setters where applicable for those values. One example of a break in this logic is in AfkStatus.class. There is no getter for the class property. Additionally, by convention, the class variable should be named 'afk' and the getter for a boolean should be named 'isAfk'. Don't use a getter naming convention for a variable name.
• In AfkStatus, for code clarity, ensure you add a space between true and ? in this line this.isAfk = this.isAfk == true? false : true;.
• There should not be a static instance of TeleportManager inside of Locations.
• The teleports variable in the TeleportManager class should be marked as 'private static' as we would not want multiple instances of the overarching teleports list.
• Every method inside of TeleportManager should be marked as static and should be stateless to the rest of the application. It appears that they are stateless already, so that is good.
• Add a cleanup method to TeleportManager which will remove any requests from the ArrayList with an expired date.
• Create an event listener class which listens for the FiveMinuteEvent from core. Inside of this event listener, call the TeleportManager cleanup class.
• Change the teleport expiry to be 1 minute from time it was sent.
• In TeleportRequest remove the class property location, it is unnecessary. Is this a result of copy and pasting code? If so, at least ensure you are reading over the logic after copy and pasting.
• Remove the default constructor from TeleportRequest as it would be undefined behavior to create a blank TeleportRequest
• Remove the setTeleport method from TeleportRequest, and instead move the logic to the initializing constructor. We shouldn't be changing a teleport request after it's already been created, and thus the method is unnecessary.
• The Tp class has incomplete logic. It never actually TP's someone.
• If the following changes above are implemented, the following shouldn't compile anyways TeleportManager teleportManager = RRPGEssentials.locations.teleportManager; but, this is bad practice. Don't write code like this, where you're setting a variable without a function call on the right side. With the following changes above, the following is possible TeleportManager.teleport(player)
• PlayerData classes should never implement logic. They are a means of containerizing data only. Inside of LastLocation, remove the method public void goToLastLocation(Player p). If this functionality is needed in many places, you could add a static method to TeleportManager: `teleport(Player player, Location location).
• Invoking this event new onPlayerWarp(p, temp, currentLocation).invoke(); should only happen inside of TeleportManager. No matter if your teleporting to a home, a player, or a warp, the common theme is 'teleportation' and thus the actual action of teleporting should only ever be done from TeleportManager.
• Remove any other lines where players are teleported except from TeleportManager. In place of the removed lines, write a static call to TeleportManager to initiate the teleportation.
• Inside of Repair.class (Specifically the handle method), change variable names to be more than one letter. One letter variable names should only ever be used in the case of counters, loops, and quick code tests.
• Inside of the Heal command, don't set player health to 20 as that is not necessarily their max health. Instead, set it to their max health. Hint, look at the methods available on bukkitPlayer.
• In Fly.class, change variable names to be more than one letter. One letter variable names should only ever be used in the case of counters, loops, and quick code tests.
• In Fly.class, don't use terinary when it's not nessecary. All of the logic could be changed to just player.setAllowFlight(!player.getAllowFlight())
• In Exp.class it is desired behavior to also be able to take xp away by specifying a negative value.
• Add a player parameter to the Exp command. exp $s $n
• In a lot of the classes you have created toggle functions. Toggle functions are bad in almost all cases. Please, replace toggle functions with a getter, setter, or add/remove methods.
• In IngoredBy.class, remove all logic. By this, I mean, remove public void toggleIgnoreBy(Player wantsIgnored)
• In IgnoredBy.class, change the getter method to be named getIgnoredBy(). Convention is to always prepend the word 'get' to the variable name. To automate this process in IntellijIDEA, use the keyboard shortcut 'alt + insert'.
• In LastmessageReceivedBy.class, change the variable paraemeter name to something more meaningful than a single letter: public void setLastMessageFrom(Player p)
• In NickName.class, change the variable parameter name to be nickname instead of nick in all places it appears. Don't use abbreviated variable names for parameters unless there's good reason.
• In ChatListener, add a priority of low to the event handler. This ensures, it'll come last in the event pipeline to ensure it takes precedence.
• In ChatListener, don't make a local variable for a single function call. Remove this line: IgnoredBy ignoredBy = RRPGCore.players.getPlayerData(event.getPlayer(), IgnoredBy.class);
• In ChatListener, remove the method: public ChatListener(). An EventListener, as implied by the name, should not be in charge of registering itself. Instead, preform this logic in RRPGEssentials.class
• Add access modifiers to onPrivateMessage.class. Also, instead of extending bukkitEvent have it extend coreEvent. This stops you from having to constantly re-implement the handlers logic in all your event classes.
• This is not how you invoke events.new onPrivateMessage(player, lmrb.getLastMessageFrom(), args.get(1).getString()); Fix it. There are plenty of examples in Core, Items, and Combat to look at. This particular example is in R.class, but is found in other classes as well. Fix them all.
• In Ban.class and likely other classes, the command format does not support a parameter type of $i. Numerical parameter types are shown with the parameter type $n.