Make code clean and easy to understand

Discussion in 'Programming' started by Sh99, Jan 29, 2020.

  1. Hello guys,

    this is a quick guide to make your code clean and easy to read without changing the logic itself.

    Negate if statement

    Sometimes you may try to check if something is true/false with if statements. An example could look like this:

    Code (Java):

    public void myFunc(@Nullable Player p)
        {

            if(null != p){
                p.sendMessage("A player is available");
            }
        }
     
    What happens is that you go deeper and deeper into if statements and on a big logic or algorithm you could reach your screens end while typing.

    To avoid this you just have to negate the if statement. As you are currently checking if the player is not null you will check if the player is null and end the method with a return statement.

    This could look like this:

    Code (Java):

    public void myFunc(@Nullable Player p)
        {

            if(null == p){
                return;
            }

            p.sendMessage("A player is available");
        }
     
    Comments

    As you got the power of writing info-lines between your code-lines you shouldn't write down every single information you get. Its more a quick overview about what your code does and some basic method information such as paramets and result.

    Note it is important you don't overuse comments. A javadoc comment, which is always place above a method or class is always useful, as inline-comments only useful for complex algorithm which is hard to understand but in most cases you will not need the "//" comment.

    1. Every class needs a comment

    Code (Java):

    /**
    * A short explanation of what your class is about and which methods it contains.
    * It should be 3-5 lines of text for this description.
    */

    public class ShopCommand implements CommandExecutor, TabCompleter
    {
    }
     
    2. A method with complex logic could have a doc/comment but it is not so important than adding doc to class.

    Code (Java):

    /**
    * If you got a complex method you can also add this text to your code.
    * Its could be useful to understand better what was the intention of a method but
    * it is not that important as describing what the whole class does, so its more
    * an optional thing.
    *
    * @param sender Some more description here
    * @param command Some more description here
    * @param s Some more description here
    * @param args Some more description here
    * @return boolean
    */

    @Override
    public boolean onCommand(CommandSender sender, Command command, String s, String[] args) {
    }
     
    Layers of development

    The next thing is when you start development of a big plugin you should build up a layered structure for your project.

    This could look like this directory tree (For a developer at: example.org):

    See: https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

    Code (Text):

    src
    -----org
    ----------example
    ---------------Command
    ---------------Manager
    ---------------Handler
    ---------------Listener
    ---------------Entity
    ---------------Task
    ---------------Main.java
     
    Main.java:

    The main.java is the file where your plugins starts working in Main.java#onEnable. In your manager you would initialise all handlers and manager and do dependency injection to it. This could look like the following:


    Code (Java):

    public class Main extends JavaPlugin
    {
        private Connection con;

        @Override
        public void onEnable()
        {
            try {
                this.con =  DriverManager.getConnection("jdbc:mysql://localhost/minecraft?user=admin&password=admin");
            } catch (SQLException e) {
                e.printStackTrace();
            }

            MyManager myManager = new MyManager(new MyHandler(this.con));


            this.getCommand("mycommand").setExecutor(new MyCommand(myManager));


            this.getServer().getPluginManager().registerEvents(new MyListener(myManager, this), this);
        }

        @Override
        public void onDisable() {
            try {
                this.con.close();
                this.con = null;
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }

    }
     
    Now i want define the layers a bit more detailed.

    Entity:

    To make clearer which properties you can use of a result from database you store you result array into and object.

    For example an array like this:

    {
    "username": "admin",
    "password: "test"
    }

    Could be stored like this:

    Code (Java):

    public class User
    {
        private final String username;

        private final String password;

        public User(String username, String password)
        {
            this.username = username;
            this.password = password;
        }

        public String getUsername() {
            return username;
        }

        public String getPassword() {
            return password;
        }

        /**
         * For example to set a new password. But be careful using setter on entity
         * since you only should add this when you want to allow changing the value of a property.
         * @param password
         */

        public void setPassword(String password) {
            this.password = password;
        }
    }
     
    Handler:

    A handler connects directly to your database and returns the result in an object <Entity> or just nothing for example when inserting something into database. The handler layer is not for exception handling, you just throw the exception to the method where your handler function is called <Manager>.

    This is only an example, you need to create your own database handling. You can also just insert the Connection and run queries or statements on it.

    Code (Java):

    public class MyHandler extends SqlInitialisedHandler
    {
        public MyHandler(Connection con) throws SQLException, ClassNotFoundException, IllegalAccessException, InstantiationException, NoSuchMethodException, InvocationTargetException {
            super(con);
        }

        public void insert(@NotNull Player player, @NotNull double value) throws SQLException {
        }
    }
     
    Manager:

    The manager layer is the most important layer for your business logic. So A handler just get and put data to your database. An entity is just to now which properties you get from a handler, as a manager is the layer between something triggering your plugin and your database handler <Handler>.

    So in a manager you just call the handler function you need a form the result of it before giving it back to the top layer for example a command or a listener. You can also do exception handling to check if your query runs through or not and giving the correct response.

    A common use-case is an economic plugin. Whenever you give yourself money you will do /money add {value} for example. Now your MoneyCommand class would be triggered and call the manager which contains a ecoManager.add(player, value); method. You use it and give yourself the expected amount of money.

    Now you want to add money to a player after killing a mob so you can still use the same method of your manager (ecoManager.add(player, value);) but now in the Listener-Layer not in the Command-Layer.

    Code (Java):

    public boolean create(@NotNull Player player, @NotNull double value)
    {
        try {
            this.myHandler.insert(player, value);
            return true;
        } catch (SQLException e) {
            return false;
        }
    }
     
    Trading database connection to other plugins

    Since you made more than 1 plugin and which depend each other you should create a single plugin just for instantiating your database connection.

    So for example you have an economic and plot plugin you can create a third plugin for example named persistence.

    In this plugin you open a database connection and just call this static method in other plugins sou you only need exception handling 1 time in the persistence plugin.

    For instance you could create this method in your Main.java of persistence plugin:

    Code (Java):

    public static Connection getConnection()
        {
            try {
                return DriverManager.getConnection("jdbc:mysql://localhost/minecraft?user=admin&password=admin");
            } catch (SQLException e) {
                e.printStackTrace();
            }
           return null;
        }
     
    And access it in your eco and plot plugin like this:

    Code (Java):

    public class Main extends JavaPlugin
    {
        private Connection con;

        @Override
        public void onEnable()
        {
            this.con = developername.persistence.Main.getConnection();

           if(null == this.con){
             return;
           }

            MyManager myManager = new MyManager(new MyHandler(this.con));


            this.getCommand("mycommand").setExecutor(new MyCommand(myManager));


            this.getServer().getPluginManager().registerEvents(new MyListener(myManager, this), this);
        }

    }
     
    I hope this i helpful for someone of you. :)

    When you got any question feel free to ask i will try to answer asap.

    Im also happy to hear suggestions for this guide.

    P.S.

    As you gave me that much content/feedback for this thread i need to add all contributors to this credit list.

    Credits:

    Sh99
    SteelPhoenix
    Bobcat00
    Optic_Fusion_1
    Stef
     
    #1 Sh99, Jan 29, 2020
    Last edited: Jan 30, 2020
    • Like Like x 5
  2. SteelPhoenix

    Moderator

    This is somewhat situational. Personally I don't return and just put my code inside the if statement if I only have a few lines of code (1-3 or so).

    This does not follow Java naming convention. Package names should be lowercase and class names should be unique and descriptive. Also, Main is often reserved for the application entry point. In my opinion, one should name his main class like WorldEditPlugin.

    https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

    The fields should be final if they are not meant to be modified (e.g. the username field). Also, there is no argument validation here (think of null values, non-standard characters)

    A UUID should be saved as a BINARY(16) (it's a 128bit value). Saving it as String is really inefficient and can cause issues with character encoding. The #close() method should be put in a finally block to ensure this happens even if an error gets thrown. In addition to this, you can also use a try-with-resources block to do the closing for you.

    Final notes
    The guide needs some polishing but seems solid for people new to programming. Personally I wouldn't have gone with SQL as it's a more advanced topic and you did not mention I/O operations should be done async. Also, keep in mind a Connection may not stay open forever.

    I did see your examples lack proper exception handling (you just print the stack trace and pretend nothing happened, sometimes not even that)

    Also, for other plugins to access your plugin you don't need to use static.

    P.S.: If you want this thread moved to Spigot Plugin Developement and tagged as 'resource' let me or any other staff member know.
     
    • Like Like x 2
    • Agree Agree x 1
  3. Thanks for the feedback! I will optimize it, i also noticed that not all i said is full understandable like the SQL thing or java naming convention.

    I will try my best. so long :)
     
    • Agree Agree x 1
  4. The best way to make your code easy to read is to write a lot of comments. A lot. Each method should have a paragraph explaining what it does. Each block of code should have at least a sentence explaining what it does.

    I forget the exact number from the coding standards where I work, but I think 40% of the source file are required to be comments.
     
  5. SteelPhoenix

    Moderator

    Yes and it increases maintainability as well. I don't have any requirement but I for sure have a comment every 5 lines.
     
  6. Optic_Fusion1

    Resource Staff

    Some comments however aren't really needed.
    The example code below for example doesn't really need any comments, just a returns
    Code (Text):

    public void Player getPlayerByName(String name){
    //code
    }
     
     
    • Agree Agree x 1
  7. But it should have comments. Does it work for online players, offline players, or both? What happens if the player isn't found? What about a player having never been on the server vs a player who doesn't exist within the Mojang universe?
     
  8. Optic_Fusion1

    Resource Staff

    If we look at org.bukkit.Bukkit we have different methods for online & offline players, so we'd do something like that.
    org.bukkit.Bukkit.getPlayer(String name) && org.bukkit.Bukkit.getOfflinePlayer(String name);
    In general if you're able to name the method in a way that doesn't require a comment, do it.

    an example being Bukkit.getGenerateStructures() would be better named as Bukkit.shouldGenerateStructures() or Bukkit.shouldStructuresBeGenerated() or something
     
  9. I updated the guide for your suggestions and removed the sql part as it only should visualize how to use layers not SQL.
     
    • Like Like x 1
  10. Regarding comments I think we should note the difference between an inline comment and a JavaDoc comment. I personally think JavaDoc comments are extremely valuable in maintaining clarity. Of course the length of the JavaDoc changes per method. In my project I have a method declaration (just the declaration, it doesn't do anything) with 21 lines of JavaDoc, while there are also a bunch of getteres/setters with about 3 lines. Regardless, I think the inclusion of JavaDoc can enhance the project even when you just have three lines of them.

    Inline comments on the other hand, I'm not that much a fan of them. They can definitely be useful, but most of the time they explain what is being done, while that's usually not useful (i.e. //loops over the objects, yeah I got that already by the literal use of a for loop). Instead, they are much more valuable to explain why something is being done in the way it is (i.e. explain why it's necessary to loop over the objects from back to front). I'd argue that most of the time when you need an inline comment to understand what is happening, you should refactor your code.
     
    • Agree Agree x 1
  11. I would also add that instead of chains of ifs for things like command logic, I prefer the fail-fast approach where you check if certain values are incorrect and break out of the method as quickly as possible in those circumstances. \

    For example:
    Code (Java):

    /**
    * execute the command
    *
    * @param sender the CommandSender who issued the command
    * @param aliasUsed  the alias that was used
    * @param args the arguments of the command
    * @return false if the commadn args were wrong and the command usage should be sent to the player, false if otherwise
    */

    @Override
    public boolean execute(CommandSender sender, String aliasUsed, String[] args) {
        //can only be run by players (as defined in the BenderBattleCommand enum)
        if(args.length != 1) {
            return false;
        }

        Player player = (Player) sender;
        String benderName = args[0];

        if(benderName.equalsIgnoreCase("off") || benderName.equalsIgnoreCase("none")) {
            if(activeBenders.containsKey(player)) {
                activeBenders.get(player).destroy();
                activeBenders.remove(player);
            }
            sender.sendMessage(Lang.PLUGIN_PREFIX.getString() + "You are no longer a bender");
            return true;
        }

        if(BenderBattle.benderRegistry().isBender(benderName)) {
            if(activeBenders.containsKey(player)) {
                activeBenders.get(player).destroy();
            }
            BenderRegistry.RegisteredBender b = BenderBattle.benderRegistry().get(benderName);

            Bender bender = b.createBender(player);
            bender.initialize(projectileManager);
            activeBenders.put(player, bender);

            sender.sendMessage(Lang.PLUGIN_PREFIX.getString() + "You a now a " + b.benderInfo.displayName());
        } else {
            sender.sendMessage(Lang.PLUGIN_PREFIX.getString() + ChatColor.RED + "Unknown Bender");
        }

        checkTickingTask();

        return true;
    }
    }
    if the arguments given are incorrect I fail immediately and break out of the method call without having to be concerned about the rest of the if chain. from there I parse the arguments into variables that the rest of the method will use and if parsing could fail for any reason the method can simply abort immediately. No if/else chains, just simply a sequence of events that under certain conditions are designed to abort the rest of the command.
     
    • Like Like x 1
  12. That is what i already mean with "Negate if statements", i will try to adjust this :D
     
    • Like Like x 1
  13. Where I work, coding standards prohibit multiple return statements, except in a few very limited circumstances.
     
  14. interesting. I abuse multiple returns constantly -- especially in text command parsing. Though I could see how limiting to a single return and instead mutating what is returned could have some benefits
     
  15. Great tip. Overused if and switch statements are awful when there’s a ton of things being compared
     
  16. Don’t spam comments though, I think only add comments if the function is big or does a task that is not clear at first sight.
    I am assuming you won’t forget what simple tasks do or others won’t recognize
    Code (Text):
    Bukkit.getWorlds().get(0)
    what that does.
    I say keep function and variable names short and not misleading.
    don’t make functions too large, split it into more functions and add comments to each step if necessary.
     
    #16 retrooper, Feb 22, 2020
    Last edited: Feb 22, 2020
    • Agree Agree x 1
  17. Your code would not be accepted where I worked.
     
  18. Considering you're already using @Nullable, I'd recommend going the extra mile and explicitly annotating all your parameters, and the method itself, as Nonnull/Nullable. Not to mention making your parameters final, for utmost clarity. Immutability by default greatly improves your code's readability, and may even have benefits when debugging.

    Considering how you've got a link to the naming conventions here, it's incredibly ironic how you somehow have incorrectly named all your packages, and the main class name. Package names should be lowercase, and ideally a single word. Abbreviate if otherwise, or find a better name.

    In my opinion, this is blatant disregard of the Single Responsibility Principle (SRP). If you've never heard of this, it's quite simple. As the name suggests, things (classes, methods/functions, etc) should have a single responsibility. For example, a main class, would serve as the application's entry point exclusively, signalling the individual components of that application to begin their initialization process. In your example, your main class appears to first initialize a jdbc connection*, define a manager which accepts an ambiguously named "handler" (seriously, don't use these two words in the same project), register a command, and finally register an event. While the latter 3 are somewhat (unfortunately) acceptable, the first simply isn't. A connection initialization has absolutely no place whatsoever in your main class.

    * - On an unrelated note, you're wrapping this connector in a try/catch, and then simply printing the stack trace - not even stopping the plugin after. Rather useless if you ask me. For starters, don't just print the stack trace, actually handle it. Also, don't print the stack trace manually regardless (via Exception#printStackTrace)! Pass the exception into your logger, and let it print the stack trace.

    Be really careful when making objects like this. While it's not a problem with the example provided, imagine a far more thorough user object, that for example, contains fields for the username, password, location, timezone, pronouns, first name, last name, age, etc. At this point, a constructor would be incredibly confusing to manage, with all the different required parameters, and conflicting types. As an alternative, consider the builder pattern for manually constructing your objects.

    Considering this specific example though, where you've got your deserialized data, and your object, automatic object mapping is usually an infinitely better alternative. Gson's object mapper in particular, is great, and extendable to virtually any data format that can be serialized into a map-like structure. As a benefit, gson is already bundled into spigot, so no extra dependencies needed!

    Also just as a side note, this example is logically flawed. Your password field is immutable, yet you've got a setter for it.

    I already briefly went over this, but suffixing different functional components with "handler" & "manager", is extremely ambiguous. I can prove this, by simply stating the definition of each:
    Manager: a person responsible for controlling or administering an organization or group of staff.
    Handler: a person who trains or manages another person.

    Once you decouple these definitions from the "recipient", as far as I can tell, the meanings are identical. Looking at your handler class, an appropriate alternative could be EconomyDatabase.


    Now, I've spent the majority of this post complaining, so here's my praise for what you did correctly mention:

    Thank you for providing that manager example, where you've decoupled the business logic from the data object. Too often I see data objects with business logic in them, and while afaik, conventionally, there's nothing wrong with this, I find that it usually makes for a clusterfuck of a program. The separation of the business logic from the data object, seems to improve maintainability greatly.

    Thank you for mentioning javadocs, I believe they're incredibly important. I've personally been trying to get into better habits regarding these - pretty much all the work I've ever done is undocumented.

    Don't ever use a finally block for this, it's not guaranteed they'll get executed. Always use try-with-resources for autocloseable objects.
     
    #18 PiggyPiglet, Feb 22, 2020
    Last edited: Feb 22, 2020
    • Agree Agree x 1
  19. SteelPhoenix

    Moderator

    Can you link me any sources? afaik the only issue is that exceptions thrown in the finally block masks the original exception.
     
  20. This is a very well put guide.
    For me, an amateur developer still in learning, following courses and putting helpful comments and notes into your programs helps for referencing and has really boosted the speed of my improvement.