Beginner Programming Mistakes and Why You're Making Them

Discussion in 'Spigot Plugin Development' started by Choco, Oct 14, 2017.

  1. so this is a bad way?

    Code (Text):
    public class Main extends JavaPlugin {
        private static Main instance;  
       
        public void onEnable() {
            instance = this;
        }
       
        public static Main get() {
            return instance;
        }
    }
    and this for other classes?

    Code (Text):
    public class OnMessage {
        private static OnMessage instance = null;

        public static OnMessage get() {
            return instance == null ? instance = new OnMessage() : instance;
        }
    }
     
  2. Maximvdw

    Benefactor

    Just because you are not planning to extend something now doesn't mean you never will. I've originally created FeatherBoard (anti flicker) in August 2014 for a server. It only had to do specific things and freelancing meant I had to get things done quickly. Eventually that server never paid, I had a good idea and I wanted to sell it on spigot. In my rush I indeed had the mistake of creating a "Scoreboard" class with static map containing players and scoreboard instances...

    I had no intention to change that, of all possible scenario's there was no scenario in my head that said why I would need to waste more time on it to get it fixed.... until protocol hacks and things like viaversion became a thing that actually required to send different packets to different people.... and I created the thing I had to do long before that - some centralized place to give everyone a scoreboard. It was a big change because changing all of that caused a lot of testing ... people are impatient, so you build up technical dept to get the update finished as soon as possible.

    What I want to say, its best to do things right - separate as much as possible before you release a plugin - because once you publish it, people will be impatient for updates and fixed.. it will cause you sleepless nights or bad reviews
     
    • Informative Informative x 2
    • Agree Agree x 1
  3. Maximvdw

    Benefactor

    1) is bad because there is literally no use for it to be a singleton. Plugins like plugman can even break those things. You should instantiate classes that use the main plugin with a reference to the Main instance.

    2) As a singleton it is not bad, but I have no idea what OnMessage does or why it needs to be a singleton because its name looks like a method name.
     
    • Agree Agree x 1
  4. To be fair, NMS/CB/Bukkit/Spigot/whatever layer is doing this pretty much already turns your main class into a singleton for you without doing anything. With that I mean, provide an access point to it via a static method (MyMainClass.getPlugin(MyMainClass.class)) and block instantiating the class again for the second time (calling the constructor will throw an error iirc). Even if you don't want this, it's pretty much forced upon you. You can of course use DI, but then you end up with a Singleton you pass around with DI and I'm uncertain if that's desirable either.
     
  5. Maximvdw

    Benefactor

    the problem is that it can be broken by loading the plugin with another class loader
     
  6. Wouldn't that be the case with every Singleton?
     
  7. MiniDigger

    Supporter

    Depends on how the Singleton is implemented. Guice Singletons are thread safe.
     
  8. 42 is always constant, its the answer to life the universe and everything...

    Good post. It deserves a bump every 10 months or so. ;)

    My wife is a librarian, she says anything that gets a kid to read is good.
    I'm a programmer, I say anything that gets a kid to program is good.
    There are books you can buy about how to make a minecraft plugin, my wife's library has a copy because i donated it. Don't hate the beginners, help them.
     
    #48 Tarluin, Dec 1, 2019
    Last edited: Dec 1, 2019
  9. Explain why is it bad to abuse static, i dont see the problem
     
  10. static is a way to define that fields and methods belong to a class, not an object like usual. Sometimes, however, people use it as an access modifier: a way to easily access their code across classes. Often this completely defies any kind of object-oriented programming - the ideology being that you should create objects and then access contents from within these objects. Since Java is an OO programming language, this usually has a lot of downsides as you're actively working against the basis of the language. A whole suit of issues occur: poor readability, poor maintainability, being prone to errors among others. A better way to go about it is usually to try and design your program in an object-oriented manner and you'll notice that you often don't need the static keyword to achieve what you want.
     
    • Like Like x 1
    • Agree Agree x 1
  11. Abuse is always bad. It's in the very definition of the word :p

    Stef already explained from a high-level perspective what abuse means here, but I'd like to dig a bit into how the keyword is useful and where it is appropriate to use. It can be really difficult to understand why it exists if it's only "bad".

    You probably already make heavy use of static methods. The general rule of thumb for static methods is that they should be pure functions. A pure function is one that doesn't cause or rely on any side effects. An example of a pure function is Math.abs(). This function returns the absolute value of a given number. It makes sense for this function, and indeed all* of the functions on java.lang.Math to be static, because they are pure. They just take an input, do a calculation based on nothing but that input, and produce an output that is returned to the caller. If the methods weren't static, you would have to instantiate the Math class to call them, and there's literally no reason to do that, because there is no benefit of putting pure functions in objects. Java doesn't allow you to define functions outside of classes, so they have to belong to some class. Such a class, full of pure functions, is called a utility class. Utility classes are fine. Some will argue that they make other code harder to test, and while that is true, limiting utility classes to well-defined functionality that never varies (you don't need two different implementations of Math.abs()) can usually help mitigate that. Utility classes also work fine in Detroit-style (bottom-up) TDD, but they can be a little frustrating to work with in London-style (top-down) TDD, because they are difficult to stub.

    The static keyword can also be used on fields, and that's where things start to get funky. Static fields are global state, and global state is harder to reason about than local state. When I say "reason about" it's what Stef alludes to with "readability". Given a piece of code, can you explain what it does? Now, every time you declare a field (public) static, it becomes global state and thus accessible to the entire program regardless of how relevant it is to any given part of the code base. This is fine for constants like Math.PI, because they never change. Whether you tediously type 3.14159265358979323846 everywhere or reference the Math.PI constant is the same, semantically, but it's really hard to remember all those digits, so that's why it is defined as a constant. It gets funky when the values of those static fields can change, because you no longer know exactly what value you're dealing with at any given part of the code base, because it can be changed from anywhere, unless you specifically "lock it down". This is what the Singleton pattern is about (see Math.random() JavaDocs, linked below). As Stef mentions, people use the static keyword on fields as if it was an access modifier, because they don't know how to pass objects around in their code. They don't use it for its intended purpose, but it solves their problem, so they apply it everywhere. It becomes a really big problem if you ever need to have two different instances of that something (you can't), or if you ever want to write automated tests for it (you have to set up all the static fields before running the code you're testing).

    I could go on and on, and bombard you with code samples, but the truth is that you won't truly understand what constitutes static abuse until you've seen it and experienced the issues that come with it. It's just something that will eventually click with experience. The moment you start writing your own unit tests and have to call a bunch of static init() methods to set everything up, remember that testing can be easy and straightforward with proper software architecture. Or when you suddenly need two or more separate instances of something and you have to rewrite hundreds of lines of code, pay attention to how many places you have to remove the static keyword to make it work.

    Hope that helps a bit. Happy hacking!

    ---

    * Math.random() is an exception. Notice how the JavaDocs explain that it actually causes a side effect in that an instance of java.util.Random is created and cached (the Singleton pattern) for future invocations.
     
    • Like Like x 3
    • Agree Agree x 1
    • Informative Informative x 1
  12. I think if you have a really basic plugin, say all it does is disconnect a player if they use the wrong ip, then you shouldn't be trying to make it a bunch of classes. A plugin that basic really only needs to be 1-2 classes.
     
    • Agree Agree x 3
    • Funny Funny x 1
  13. drives_a_ford

    Junior Mod

    I was just wondering.

    Perhaps the "I'll fix it later" mentality could also be addressed here?


    Many people seem to say (or at least allege) that they know that their code is not well written and insist they'll fix it later.
    While I'm sure most such people actually do intend to rewrite their code in a better way when they say such things, I'm afraid few actually do in the end.

    I'd say there are four main issues with the "I'll fix it later" mentality
    • Most never end up fixing it
    • Bad practices lead to actual problems
      • Ones that could be avoided by adhering to the principles
    • Bad practices lead to more time spent on projects
      • Because the projects are hard to maintain
      • If they do decide to rewrite it, that'll be extra time as well
    • Using bad practices creates bad habits
      • When starting off every project writing code that doesn't adhere to proper principles one creates a habit of writing such code
      • It's difficult to get rid of a bad habit
     
  14. MiniDigger

    Supporter

    "I know this code is SQL injectable, ill fix it later"
    Famous last words
     
    • Funny Funny x 2
  15. Thats very helpfull. Thanks :D
     
  16. Static factory methods? Pretty commonly used, and pretty commonly useful
     
    • Agree Agree x 2
  17. Making everything static is also both common and subjectively useful :p

    Static factory methods are not any different from any other kind of static access in terms of coupling. You're still creating a direct dependency, and transitive dependencies to whatever the factory method depends on. While the new keyword often reveals most of the transitive dependencies, static factory methods usually hide them (that's one of the more common reasons for having them), because they're typically provided for convenience. They can be useful in bootstrapping code and other "fixed" parts of a software system, but using them willy-nilly in your code base will create tight couplings that rob your components of testability and composability, just like other forms of static access.

    I will say, though, that factories - in general - are a very, very useful concept in any sort of framework. They can help bundle up complex functionality behind a simple interface, and they can create a very specific kind of flexibility that's hard to arrive at by other means. But static factories carry the same kind of "code smell" as other forms of static access for the same reasons. At the end of the day, any sort of software pattern is situational, and the same goes for factories - both the static and the non-static ones.

    A fun little question I like to ask when discussing various programming concepts, especially in the context of beginners, is: "How much bad code do you think this concept will allow someone to write?". Proper application of static factory methods is arguably a fairly advanced topic, and I'd wager giving someone that hammer without them understanding the why is bound to result in more stuff broken than built, so I'd still throw that in the "abuse" basket, if only to be on the safe side :)
     
    • Like Like x 1
    • Agree Agree x 1
  18. Choco

    Moderator

    Couldn't have said that better myself.
     
  19. The problem with a lot of practices is that the majority fall into the "it depends" category. I have a codebase where the factories are static and have a codebase where even the factories have inheritance (factories that extend factories). Both of the options aren't necessarily wrong, it just depends on what you're trying to achieve. A lot of the time these practices are helpful, but there's also a point at which you have to ask yourself whether or not this is overkill. You can throw every practice at everything and suddenly your very simple class ends up having both an interface and an implementation, a factory, a builder, etc. Static factories can be used correctly and incorrectly and that is the case for almost all design principles.

    Without disrespecting the points Choco, garbagemule and others have made in this thread, since they pointed out very common pitfalls and reasons as to why these are pitfalls, I think you're in general better of asking yourself if it makes sense to do something like x in your code. Is it necessary? Does it provide some safeguard? Does applying this make it harder to cause bugs? Does this make your code more extensible in the long run? If it is more extensible, doesn't it allow you to break the code more easily? Does this make your code more readable? Ask yourself such questions and then draw your own conclusion. Of course this requires a lot of experience and I'd argue that this is the hardest part of software development. The programming language is formally described and the algorithms have been proven and laid out, but the design doesn't have formal specifications or proofs. We have guidelines, but these are just guidelines, not some holy grail that you can throw at everything you want and will give you the perfect answer. The design has to be considered on a case-by-case basis and in the end there are multiple, perfectly valid ways to design the same piece of software.

    So, are static factories good? Look at your code, compare the designs, compare the pros and cons, perhaps ask for a second opinion and then decide whether your factory should be static or not.
     
    • Like Like x 1
  20. There's a very important point to be made about the target audience and their skill level; beginners need rules, experts transcend them. Beginners have a tendency to go apeshit with Maslow's Hammer every time they learn about a new concept. Like you mention yourself in the second paragraph, it takes experience - a lot of it - to understand when and where to use which tool, and in software development, there are lots of tools and a vast, endless ocean of experience to absorb.

    The biggest issue with asking these questions, let alone telling someone to ask themselves, is that answering them requires knowledge of the cons and the drawbacks, because otherwise it's just always "yep, looks fine, moving on". Seasoned developers will break all sorts of rules all the time, exactly to avoid the cascading over-engineering that following the rules would result in, but they're "allowed" to do so because they understand the consequences, and know how to backpedal if it becomes necessary. Think of the difference between writing short little bash scripts on your computer for trivial tasks vs. pushing changes into a production environment. Beginners won't necessarily know the difference other than "one is dangerous, so look out"... But look out for what, exactly?

    Circling back to the target audience of this thread, I think it's perfectly fine to say that static factory methods should be avoided. By the time you understand how to properly use them, this thread isn't for you anymore anyway, and you'll probably also understand why you were advised against them to begin with (if not, you just have some more mistakes to make, and that's fine).

    There is a really nice talk by Dan North about effective team patterns in which he covers the Dreyfus learning model, which is highly relevant to this thread and this discussion in particular. He puts a twist on it to make a point about pair programming, but the model is really well covered. Check it out here (from around 5 minutes in until about 12 minutes in, but I highly recommend the rest of the talk and Dan North in general, he's awesome).
     
    #60 garbagemule, May 3, 2020
    Last edited: May 3, 2020
    • Like Like x 2
    • Agree Agree x 1