1.8.8 Rate my code

Discussion in 'Spigot Plugin Development' started by ZevoGaems, Mar 30, 2020.

  1. I am working on ananticheat base, the code for the base was made quickly and I would like some feedback if the it is bad/unreadable/non scaleable. Thanks for any feedback =)

    https://github.com/LoopTurn/DayBreakAC
     
  2. I like your code structure, but I have a few minor notes from a quick glance:
    1. Your permissions in files such as this should be typed as `final` and then the variable name should be capitalized.
    2. I'd store your interfaces in a separate package. Keep your "workers" and "types" separate, if you get my drift.
    3. The onCommand method in this file is sloppy. Fix up your formatting.
     
    #2 View, Mar 30, 2020
    Last edited: Mar 30, 2020
  3. SteelPhoenix

    Moderator

    Fields should be marked final when possible.
    You don't follow Liskov's substitution principle
    You should use primitives and not their wrapper classes
    A plugin's main class named 'Main' is not unique nor descriptive
    There is no reason to use new String(String)
    Overridden methods should be annotated with @Override
    There is no reason to check the command label when using the command executor for *one* command set using PluginCommand#setExecutor()
    In the GUI command class there is no reason to create a loopturn.daybreak.core.GUI instance before checking if the sender is a player (because you won't use it if this is not the case).
    Your package naming is weird

    Code (Java):
    public static void sendCommandSender(CommandSender sender, String message) {
        sender.sendMessage(ChatColor.translateAlternateColorCodes('&', message));
    }
    public static void sendPlayer(Player p, String message) {
        p.sendMessage(ChatColor.translateAlternateColorCodes('&', message));
    }
    This overload is not needed.

    Code (Java):
    public static CheckManager getInstance() {
        if(checkmanager == null) {
            checkmanager = new CheckManager();
        }
        return checkmanager;
    }
    The instance field should not be null so there is no reason to check. Also, lazy initialization can cause concurrency issues.
     
    #3 SteelPhoenix, Mar 30, 2020
    Last edited: Mar 30, 2020
    • Agree Agree x 1
    • Informative Informative x 1
  4. Thank you for all of the suggestions, I will probably re-code it to go slower and get the final and stuff in there due to some other stuff. I am also not particularly fond of my code structure but I guess it woks. Thanks again for all the great advice =)
    How would you say I fix up the onCommand class? I know it is very messy but I don't know what I can do to clean it up.

    Thank you for the advice. I will have to do some research on Liskov's substitution principle. All of that advice is a lot so I will probably re-code it keeping that in mind. I don't know when I used override, maybe for my onCommand if that is what your talking about but I don't really use inheritance much. Also that GUI thing I fixed but I will probably recode anyways. And I don't know good package naming, I will look into. Thank you for the advice =)
     
  5. 1. Don't call your JavaPlugin class "Main", this is totally misleading. Instead, use the name of the plugin itself.
    2. Log.sendConsole(new String("Starting %prefix%...").replace("%prefix%", prefix)); logging the plugin enable and disable is kinda useless since spigot is already logging it to console for it, so the message will appear twice.
    3. private String prefix = "DayBreak Anticheat"; should be final (and anything else that you do not plan on modifying later, including List and HashMap).
    4. new String("%prefix% started!").replace("%prefix%", prefix) - Why not just use directly prefix + " started!"??
    5. private void loadAntiCheat() use appropriate name for your methods and your classes. This method doesn't load the anti cheat itself, onEnable does it. This one only registerCommands.
    6. Just noticed that your package names aren't following the convention - https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html
    7. In your class Log, why not just one name for all the four methods : sendMessage ? Just use different parameter(s) for each of them so that way it is more convenient when coding to use Log.sendMessage(message); or Log.sendMessage(receiver, message); directly
    8.
    Code (Java):
    if(!(sender instanceof Player)) {
                Log.sendCommandSender(sender, "&cYou must be a player to use this command!");
            }
    You do not use return; so the console and any command block sender can still use this command and it's going to throw some exceptions.

    I didn't take the time to check everything, but these are ways to improve your current code based on my observations :) Good luck and keep up the work!
     
  6. It's a github repository, so you can see a rating using codacy.com