Update readme to have less bad practices

[AllieQpzm]

As I'm sure you're aware (being the creators of it) Vault is a very useful thing. As such, beginners often end up using Vault for their plugins fairly early in their development careers. Unfortunately, the example plugin found in the readme file has a lot of bad practices, which new developers inevitably end up copying, thinking it's the right thing to do if Vault is suggesting it.

For a fair while now, I've been correcting these beginners on their bad practices when I see them ask for help with their plugins. This I've been doing for a while, but I've finally decided to attack the problem at its source. Hopefully by removing these bad practices that are oh so common right now, less people will end up using them and then everyone wins! :)

Quick summary of changes are outlined in the commit message.

[Kainzo]

Lets not be insulting about it.

I don't think this is the source of the problem.

[csh]

This is more laziness than necessity by far but would it be a good idea to possibly include the lombok @Getter annotation for the sake of "noob" friendly code? I just remember that I found the project helpful when I was starting out and I still happily use it.

[AllieQpzm]

@Kainzo I'm not saying that Vault is the source of the bad practices displayed here altogether - that would indeed be insulting and would also be wrong. However, it's undoubtedly the source of new people using bad practices while using Vault - as I already said, new users copy it thinking it's the right thing to do. Changing it will reduce that problem considerably.

@albioncode I didn't include lombok, since I know a lot of people (myself included) don't use lombok, and some wouldn't recognise it. Whereas everyone has the ability to use simple getters :)

[Sleaker]

@albioncode - using examples with lombok isn't great. annotations make java code illegible, and create a barrier of entry to the standard syntax.

[csh]

annotations make java code illegible

Hmm, I'm afraid I can't relate, always found it did quite the opposite personally.
But glad to have got your input 😛

[AllieQpzm]

@Sleaker I'll agree with the lombok being a barrier thing. How about the PR itself, though? Any objections, likely to pull?

[Sleaker]

@AdamQpzm - yes, the preference was on using static access due to the way that Plugins function in the bukkit landscape. It simplifies access by allowing calls without needing to pass an instance of the plugin everywhere in your plugin. Further, I don't see what the 'bad practices' this is trying to resolve are. Instead of giving a general 'this resolves bad practices' I'd be more inclined to pull if there was an actual description on why you think there are bad practices here.

[AllieQpzm]

@Sleaker As I said, there is a summary in the commit, but I'll happily outline some here.

The use of static is widely regarded as bad practice in this sort of case. Static should only really be used in cases for when it doesn't make sense for the variable (or method) to be tied to a specific instance of something - for example, constants or utility methods (see the Math class for an example of this). This is not one of those cases - it make sense for them to be tied to the instance of the plugin. More about why static should be avoided can be seen in other posts, such as this detailed one on stackoverflow: http://stackoverflow.com/a/7084473

I also removed the Logger field. There's no real reason to be 'borrowing' Minecraft's logger, as Bukkit provides a method in JavaPlugin (getLogger()) to return the PluginLogger for that plugin - among other things, this will automatically prepend the plugin's name.

My final change was also a simple one - getLabel() returns the label that the CommandSender used. Through the use of aliases, this command label may not match the name of the command. Using the getName() method supports the use of these aliases (as the name does not change regardless of the label used), whereas getting the label does not.

[Sleaker]

@AdamQpzm - while I understand the intention, there isn't any issue with them being static. Not in the way that plugins are utilized, and how the setup has to happen for it to function. It's a utility and attempting to encapsulate it onto the actual plugin just provides a layer of abstraction that causes you to need to pass around the extra object everywhere. This is very much an example, and open to personal coding interpretations.

I don't really have any problem with the other changes, as they are there due to stuff that has changed in bukkit.

[AllieQpzm]

@Sleaker Well the problem is that this disregards any of the 'security'. Anything can access a public field, and anything can change it. Public fields should really only be limited to to constants, and something can't be considered a constant unless it's final. Obviously you can't mark them as final here.

I disagree that they should be static too. You're right it is an example, so would it not be better to go with a design that many don't view as a bad one? Users will of course be able to choose their own designs once they have learned more about the language itself.

[cerealcable]

@AdamQpzm Isn't that just a bit of fear mongering though? At some point if you're just letting anyone put a plugin on your server then you've got a whole new problem to worry about. Arguing about the access control modifiers is really nit picking and probably will never become a problem for those new to writing plugins.

Sure, there could be something someone could do because of a public static final, and honestly a different modifier wouldn't necessarily be a bad thing to do. Protected or private would be just as great as honestly no modifier at all as well.

Largely, it probably will never matter though, lots of programming languages don't even have the concept of access control modifiers and everything is treated as "public" and yet there are no inherit problems with it. I'm speaking to python, because I've been doing that lately, and yet there really isn't even a need for it.

tldr; Ultimately security lies in the administrator of the server more than it applies here, though your suggestions aren't bad, they don't change much of anything.

[AllieQpzm]

@cerealcable Understand your point, but that's why they're called "bad practices". in reality, access modifiers don't provide all that much real security - with reflection, it's perfectly possible to access a field that is marked as private. However, simply abandoning access modifiers or using the most visible one isn't really the way forwards. There are disadvantages, but I can't really think of any advantages other than a developer being lazy. Having a look through the implementation of Vault, I noticed private fields, but I didn't notice the public fields. Why, then, should the example plugin be any different?

While it may not be any real 'security', one thing it does do is that it expresses intentions. By having a public field, you're advertising to any developer that sees upon it as "this is for everyone - use it, change it, do whatever you want with it." Of course, this isn't what you want for plugins. Changing the value of these fields will undoubtedly break things. Having a private field with a getter, but no setter, tells developers that the field is only for using, not for changing. This doesn't work just in fear of other developers changing your work, either. This protects against accidental changes. It's surprisingly easy to accidentally change a value and, since it'll compile fine, not notice it until error happen at runtime. Accidentally changing a private field that you only have a getter to isn't all that easy.

[Sleaker]

@AdamQpzm - mostly due to the fact that in a plugin the only way to get access to methods/fields is by actually importing that plugin's classes or by the non-existent reflection case. This isn't something that's going to happen.
I've also never come across the issue that a getter is supposed to protect against in conjunction with Vault. Regardless of the access types though, the field is no longer static, and that's the main usefulness here as it's supposed to represent global state.

[1Rogue]

Static has understandable usage in the example, though having a public, mutable field with global state is just asking for trouble. The real thing to consider here is that Vault is viewed by hundreds, if not thousands of new developers, many of whom do not know what is a "good practice" and what is a "bad practice". A new developer will see that Vault is suggesting to do something along the lines of:

public class MyNewPlugin {
    public static Economy econ; //my economy field
}

Where, if I felt like being a dick (or if I was just an inexperienced developer who didn't know better), I could easily end up doing something like:

MyNewPlugin.econ = null;

which could take hours to debug and find. Which is the exact reason the class should have control over it's members, not some random abstract class outside of it.

There's a general principle in OOP design that you should create objects and managers which are as hard to modify and change as possible. Promoting the use of an accessor/getter would potentially help newer developers build that core idea. And here's the fun part, getters don't have to be bound to an instance, you can easily still use static (as it can easily be logical to use):

public class MyNewPlugin {

    private static Economy econ; //now private, won't be accidentally manipulated

    public static Economy getEconomy() {
        return MyNewPlugin.econ;
    }

}

In the end it's best to teach people ways of programming that won't hurt them down the line with Antipatterns and unmaintainable solutions.

There is additional reasoning as to why public (and mutable) fields are a terrible idea, not to mention for API contracting, but it's irrelevant to this case other than you will be teaching new developers to think that this is okay to do (whether your intention or not).

[1Rogue]

Additionally, it might be worthwhile to further the example to show that you can use the returned service provider in any way you wish once you've obtained it, but those details are more up to whomever is pushing examples upstream.

[Sleaker]

@1Rogue if you look at the discussion I already suggested the change you wrote about, but it hasn't been done in the PR.

[1Rogue]

@Sleaker from reading it, it seemed like you were suggesting that there's nothing wrong with a publicly mutable field.

IMHO explaining what a "RegisteredServiceProvider" is would be beneficial as well (*pokes @AdamQpzm *)

[csh]

IMHO explaining what a "RegisteredServiceProvider" is would be beneficial as well

Although the name does seem obvious I feel as if understanding what they're tampering with may encourage "junior" developers or at the least make them feel less intimidated, nice shout @1Rogue

[AllieQpzm]

Well if the PR wouldn't be accepted as is then I'm happy to change it to a static instance - at least then the publicly mutable nature of it would be removed.

[1Rogue]

@AdamQpzm you can commit more to your branch and add it to the PR.

[AllieQpzm]

@1Rogue Yeah I know, just wanted to check that they want me to go with that before doing it - no point in doing it if they still wouldn't want that either :P

[Kainzo]

My head exploded after reading this.

[1Rogue]

@Kainzo anything in specific causing headasplosions?

[Sleaker]

@AdamQpzm - I'll pull in if you want to just make it private static and add an accessor.

[Sleaker]

conflicts with recent readme change and looks like this was abandoned anyway.