1.12.2 Is this code good?

Discussion in 'Spigot Plugin Development' started by TaskID, Oct 28, 2020.

  1. #1 TaskID, Oct 28, 2020
    Last edited: Oct 30, 2020
  2. Maybe change
    switch (args.length) {
    by if(args.length == 1 || args.length == 2)
    I think switch here is overkill
     
  3. I don't know much about Java standards (I'm a webdev guy), but I like to avoid nested if statements when I can.

    Code (Text):
    if (sender instanceof Player == false)
    sender.sendMessage("§cYou need to be a player to perform this command!");
    return false;
    }

    // Rest of the code
     
    #3 Noni, Oct 28, 2020
    Last edited: Oct 29, 2020
    • Agree Agree x 1
  4. Well I would advise changing it quite a bit. Some of these might be my personal preference and this list is not
    exhaustive, but to give you a direction:
    As stated by others, the main switch statement is overkill/makes the code harder to read. Instead simply check
    Code (Java):
    if (args.length == 0 || args.length == 3)
    It's good that you try to incorporate try-catch into your code, however your code for getting the target doesn't actually work.
    Bukkit.getPlayer(...)
    will not throw an exception when the player is not found, it will return null. So I would do the following:
    Code (Java):
    Player target = Bukkit.getPlayer(args[1]);
    if(target == null) {
        player.sendMessage("§cThe target is not online.");
        return true; //returning to exit is really good, prevents your code from getting unnecessarily indented.
    }
     
    Your second switch statement is not a good idea. Yes, normally when you need to go through all options for something like a string and set a variable based on that is where you would use a switch statement. You can make the code much cleaner however, by using the fact that your arguments match the enum constants, like so
    Code (Java):

    GameMode gameMode;
    try {
         gameMode = GameMode.valueOf(args[0].toUpperCase());
    } catch (IllegalArgumentException e){
         player.sendMessage("§e" + args[0] + " §cisn't a valid argument!");
         return true;
    }
     
  5. No this makes zero sense. If the sender is not a Player you cast it as such? That will always break of course. That said your point about nesting statements is true, bit all considering the indentation is not that bad compared to other points of improvement.
     
  6. Thanks, I changed the first switch statement to an if statement, but I guess I need to clarify something here:

    Code (Java):
    try {
        target = Bukkit.getPlayer(args[1]);
    } catch (Exception ex) {
        target = player;
    }
    if(target == null) {
        player.sendMessage("§cThe target is not online.");
        return true;
    }
    This is working, because if there is no args[1] given, it just sets the target to the player itself (because it'd throw an Exception), so that you can do /gm 1 to set your own gamemode. If a target player is given, but not found, "target" may be null and gets checked by if(target == null).., so everything is working as it's supposed to be.


    Code (Java):
    GameMode gameMode;
    try {
         gameMode = GameMode.valueOf(args[0].toUpperCase());
    } catch (IllegalArgumentException e){
         player.sendMessage("§e" + args[0] + " §cisn't a valid argument!");
         return true;
    }
    I could use that, yes, but I want to be able to use "0", "1", "2" and "3" to set the gamemode too, so that I don't have to write "creative" or soemthing.
     
  7. I see I misread the purpose of the try catch statement. Now that I understand want you want to do however, I would change it. Using a try catch here is very random (even so much I completely misunderstood what you want to achieve here) and makes the code unnecessary hard to read. Try this for example (you can also do this using if statements instead of ternary operator, I prefer the latter):
    Code (Java):
    Player target = (args.length == 2) ? Bukkit.getPlayer(args[1]) : player;
    if (target == null){
       player.sendMessage("§cThe target is not online.");
       return true;
    }
    Even with the numbers you could still do it by parsing from the enum. I don't think it will be much clearer though indeed.
    Another thing I just noticed and didn't really emphasize in my original post: prevent unnecessary nesting. Instead of doing an if-else for checking if the sender is a player, return if the sender is not, like so:
    Code (Java):
    if (!(sender instanceof Player)){
       sender.sendMessage("...");
       return true
    }
    //continue code here, now without having nesting which makes it a lot cleaner
    You could also use the same principal for checking permissions. This makes it so the bulk of your code is actually at main indent.

    If you want more tips:), please send a snippet/link to the updated code, little easier to go from there.
     
    #7 3ricL, Oct 28, 2020
    Last edited: Oct 28, 2020
  8. Hi, parsing is not working:

    Code (Java):
    System.out.println(GameMode.valueOf("0").toString());
    Code (Java):
    Exception in thread "main" java.lang.IllegalArgumentException: No enum constant org.bukkit.GameMode.0
        at java.base/java.lang.Enum.valueOf(Enum.java:240)
        at org.bukkit.GameMode.valueOf(GameMode.java:1)
        at me.taskid.system.commands.GmCommand.main(GmCommand.java:153)

    Process finished with exit code 1
    Imma do it with the switch then.
     
  9. Ok, here's the updated code:
    https://hasteb.in/utumitud.java
    https://hasteb.in/raqefofo.java
     
    #9 TaskID, Oct 28, 2020
    Last edited: Oct 28, 2020
  10. Yeah of course not:), the string "0" isn't one of the values. What I meant was using the Gamemode#getByValue(int) method, but that is deprecated so I reasoned it didn't really help. The code has definitely improved! One last thing I can give you is using the ChatColor enum instead of manually hardcoding the color codes. This ensures future compatibility and makes text easier to read/interpret colors.
    So ChatColor.RED + "hello" instead of "§chello"
     
  11. Yes, but I personally absolutely hate this, in my opinion, it's so much better and clearer to use "§...", idk, but I don't like these ChatColor things ;7 But it's a more personal preference
     
  12. Strahan

    Benefactor

    Personal preference is for stuff like how brackets are done, whether or not to return after logic or nest, etc. Embedding a color character is definitely not ideal no matter what, as any future change to the color system will break the plugin. Yes, the odds are quite unlikely, but using best practices is a good habit to get into.
     
    • Agree Agree x 1
  13. Yes, but I don't think this will be changed in the near future, and if it really gets changed then I have to edit my code (if I want to update), there are so many people out there who use the § instead of ChatColor... and I think mainly because it's shorter.
    Sorry, but I won't change anything on my code in this point as long as the "old" system is supported

    Edit: I'd use it for public spigot plugins, but not for my private stuff I make for my server only.
     
  14. I would go back to a switch statement, as when you want to add aliases such as 's', 'surv', '0' to 'SURVIVAL', an if statement would become very cluttered.
    However, if you're just going to grab exactly 'survival' as an argument and not accept aliases, the if statement is the best way to go.

    I also nest my code unlike a few in this thread - it's all personal preference and doesn't have an impact except when editing your code.


    I would also like to note that you can only use GameMode elements such as 'CREATIVE' and 'SURVIVAL' (not '0') when you are parsing a gamemode from your command argument - this is why you were slapped with an exception. See the top of my reply where I talk about switch statements for aliases - personal preference and is cleaner, but all up to you :)

    EDIT:
    In addition, for chat colors, I use the translate method in the ChatColor class to translate '&' color codes. Then, you don't need 'ChatColor.XYZ' or the section symbol.
     
  15. He already went back to the switch statement as we came to the same conclusion.
    You might see code nesting as personal preference, but there is a good reason spaghetti code is refused/seen as bad practice when code is reviewed. It is harder to read, comprehend and maintain.

    The third point is not true at all. As I already explain, you can parse the GameMode from the integer value, which can see if you would look the docs. I agree that if you want many abbreviations switch statements are good. If you just wanted to show how it was possible to do it without (which is a lot better if all you have to do is convert string back to enum constant).
     
  16. Someone already brought this up, but avoid nested code as much as you can afford to. Specifically, avoid code sections which have a great horizontal length. Any modern IDE will show you a vertical line within the code editor. Good code, besides efficiency, has a standard on readability. The longer the code is on the vertical axis, the more complicated it is.

    The metric of good code is measured in how simple (read: readable) you can implemented a complicated problem. And by readable I mean readable, not just "looks pretty" - though usually readable code also looks pretty. Say, "If we are not a player, we return. If we don't have the permission, we return. If we do not have 1 or 2 arguments, we notify the sender about the proper usage and then return. If we do not have a proper target, we return. If our target is not ourself, and we do not have the permission to target other players, we warn about lacking permission and return (etc)" is a lot more readable then "If we have a player, and that player has the permission we want, and there are two arguments, and we have a target, and we have the target is not ourself and we have the permission, (...), or else we warn about lacking permissions, or else we tell the correct syntax, or else we warn about missing targets, or else we warn about missing permissions, or else we warn about not being a player."

    I am not randomly talking gibberish, this is one of the first things you'll be taught when you attend any programming class at an university.

    https://docs.microsoft.com/en-us/archive/msdn-magazine/2004/july/{-end-bracket-}-what-makes-good-code-good

    Even here you can see that Simplicity and readability were top points, among the 8 points listed here you can see that simplicity, readability, design, elegence and clarity make up five.

    https://www.quora.com/In-software-engineering-what-makes-good-code

    If you read over that, you'll find that almost every response quotes "readability" as the top priority.

    There is a special case where this isn't really applicable, and that is when you implement generic algorithms (provided this is a completely generic algirhtm, a modified variant is a completely different case!) like a depth-first search or a fibbonacci spiral disc/sphere. But in real-life, such algorithms are merely a small subcomponent of your larger system.

    I think there is a good quote for this, "Everything should be made as simple as possible, but no simpler" while the origin of this lays in physics (its a quote of albert einstein) it applies heavily to programming and should be considered as the highest rule to follow.

    But yeah, Tl;Dr: No, this is not good code. Nested code is the worst mistake any developer can make. This is not a "preference", it simply is wrong. When I worked as an assistant to my professor, this was sufficient to fail your entire assignment without getting you a single point even it was a perfect solution otherwise. It is the most common pitfall for newbies to write nested code and generally try to stretch code on a horizontal axis.

    Note: Avoid nesting as much as possible, but there exist problems which have to be nested, yours isn't one.

    That aside, Bukkit#getPlayer cannot throw an exception (I think?)
     
    • Agree Agree x 2
  17. Thank you for explaining this very in depth! I completely agree with what you say here, I was also taught that code which is unnecessarily nested/spaghetti-like will mean you immediately lose a lot of points on the assignment, regardless of the functionality of your code.

    To answer you aside, the original code used the try-catch to handle the potential IndexOutOfBoundsException on getting args[1]. I also misread this originally as it is a very strange way of doing this and therefore advised OP to simply use an if statement checking the length here.
     
  18. Adding to this, AVOID using try catch statements if there are other ways to make sure you won't be getting an exception.
    Sometimes it's part of a method to throw an exception, but in general they can be avoided.

    For instance, to avoid an ArrayIndexOutOfBoundsException you can check the length of the array. For a NullPointerException you can check if the object reference is not null, and so on.

    [​IMG]
     
    #20 MartenM, Oct 29, 2020
    Last edited: Oct 29, 2020
    • Agree Agree x 2
    • Funny Funny x 1