How to improve this code?

Discussion in 'Spigot Plugin Development' started by Kyllian, Jun 6, 2017.

  1. Hey!

    I'm working on a new plugin. And I was wondering if I made any mistakes in the code. or what to improve. everything is working perfectly fine. But I just needs points to improve my plugin.
    I can't post any timings now. But I watched my CPU usage and it didn't go up (Tested by sending 50 messages)

    Code:
    https://hastebin.com/ofedepikiq.java

    Config:
    https://hastebin.com/apawadagas.coffeescript

    Thanks!
     
  2. Code like this
    Code (Text):
    !getConfig().getString("Messages." + toSay + ".Click").equals("")
    can just be assigned to the booleans directly (Gonna edit and add more as I go along).

    Code (Text):
    String type = getConfig().getString("Messages." + toSay + ".ClickType");
    you may want to take the string from that code and coorce it to an enum, that way there are clear bounds that are set for the people using the config
     
  3. Add comments so people can easily read it, when I looked at this I got a little lost :p
     
  4. No need for comments if the code is written neater.
     
    • Agree Agree x 1
  5. How should I write it 'neater'
     
  6. ScarabCoder

    ScarabCoder Retired Resource Staff
    Retired

    You have quite lot of try/catches. It might be a good idea to use a single on for all. It also looks like you don't need to try/catch anything. And finally, never catch Exception. Also, for everything you catch, print the stack trace. You shouldn't ever have errors, unless it's something like parsing a string as an Integer.
     
    • Agree Agree x 2
  7. Try to set all the config variables as enums or final fields onEnable. Use ThreadLocalRandom instead of creating an instance of the random class. If you can declare a variable before a method, do it. Utilise methods to make your code more readable and easily editable.
     
  8. Even then, you can use StringUtils#isNumeric, something the Apache Commons library provides.
     
    • Informative Informative x 1
  9. Those try/catch are for parsing sounds/particles.
    I might be able to move those up so it will try to parse one time?
     
  10. Code such as this shouldn't be made IMO:
    Code (Text):
                                        if (type != null) {
                                            if (type.equals("COMMAND")) {
                                                text.setClickEvent(new ClickEvent(ClickEvent.Action.RUN_COMMAND,
                                                        "/" + getConfig().getString("Messages." + toSay + ".Click")));
                                            }
                                            if (type.equals("LINK")) {
                                                text.setClickEvent(new ClickEvent(ClickEvent.Action.OPEN_URL,
                                                        main.getConfig().getString("Messages." + toSay + ".Click")));
                                            }
                                            if (type.equals("")) {
                                            }
                                        }

    Generally, considering I would cache the string variables onEnable, you can then check if !String#equals, then throw an IllegalArgumentException. Just preference. Even printing a message to console for clarification purposes would be nice.
     
  11. But a config reload would be useless then?
     
  12. Well sure, if you have a config reload method, account for it. The only reason I stated onEnable is because that's when a config generally gets reloaded.
     
  13. Would it be an idea to cache all the data when the random message is specified? And then check everything on it. and then remove it.
     
  14. Huh? If you mean checking to see if the string is valid, go for it. You can do as I suggested previously as well and throw an exception if it isn't valid.
     
  15. Line 29 & 42: Add an Override annotation to the method
    Line 43: time = time + 1 can be time++
    Line 48: Variable prep2 could probably be a List instead of an ArrayList, perhaps it can be even lower. You can also remove the String from the last diamond operators.
    Line 49-51: Use new ArrayList<>(Set) isntead of this manual for loop
    Line 52: Don't use Random, but something like ThreadLocalRandom.current() instead.
    Line 52-53: Variable r (new Random()) is only used once, just move the declaration into line 53.
    Line 58 & 61: Change equals("") to length() == 0 or isEmpty(), this is probably a bit quicker. (this is really nitpicky, but whatever)
    Line 58-66: if (boolean) var = true can be changed to var = boolean. Also you only use this variable once after this, so just move the check into there.
    Line 69: This should probably be a continue statement
    Line 85 & 89: Switch the params around and you don't have to worry about NPEs
    Line 89: If statement can be an else if statement
    Line 93: If statement can be removed entirely
    Line 99-108: Double try/catch statement can be converted into one. Also don't catch Exception, rather catch the exception that can be thrown. And don't leave the catch empty, do something with the error (e.g. print the stacktrace) this makes finding errors easier.

    Overall: You have a delay in the config. Instead of running the runnable every second and waiting for the interval to be reached, set the interval to the time in the config.
    Also why do you create a new thread? It seems like the code will work without one.
    Perhaps you don;t know but you can remove these barces { } from single line statements. May be a little easier to read. Also add some empty lines between parts of your code, that makes it more organised.
    There are also a lot of variables which are parse each time, what about caching those? (didn't change this in the example)

    In case you're confused this is the code with the changes made (please only use this as a reference):
    Code (Text):

    package me.kyllian.autocast;

    import java.util.ArrayList;
    import java.util.Random;
    import java.util.Set;

    import org.bukkit.Bukkit;
    import org.bukkit.ChatColor;
    import org.bukkit.Particle;
    import org.bukkit.Sound;
    import org.bukkit.entity.Player;
    import org.bukkit.plugin.java.JavaPlugin;
    import org.bukkit.scheduler.BukkitRunnable;

    import me.clip.placeholderapi.PlaceholderAPI;
    import net.md_5.bungee.api.chat.ClickEvent;
    import net.md_5.bungee.api.chat.ComponentBuilder;
    import net.md_5.bungee.api.chat.HoverEvent;
    import net.md_5.bungee.api.chat.TextComponent;

    public class AutoCast extends JavaPlugin {

        public static AutoCast main;

        public static AutoCast getInstance() {
            return main;
        }

        @Override
        public void onEnable() {
            main = this;

            getConfig().options().copyDefaults(true);
            saveDefaultConfig();

            timer();
        }

        public void timer() {
            new BukkitRunnable() {
                @Override
                public void run() {
                        //why is this new thread needed?
                        //new Thread(() -> {
                            List<String> prep2 = new ArrayList<>(getConfig().getConfigurationSection("Messages").getKeys(false));
                            String toSay = prep2.get(ThreadLocalRandom.current().nextInt(prep2.size()));
                            String type = getConfig().getString("Messages." + toSay + ".ClickType");
                            for (Player online : Bukkit.getOnlinePlayers()) {
                                if (!online.hasPermission(getConfig().getString("Messages." + toSay + ".Permission")))
                                    continue;
                                for (String str : getConfig().getStringList("Messages." + toSay + ".Message")) {
                                    if (Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null)
                                        str = PlaceholderAPI.setPlaceholders(online, str);
                                    TextComponent text = new TextComponent(TextComponent.fromLegacyText(str));
                                    if (!getConfig().getString("Messages." + toSay + ".Hover").isEmpty())
                                        text.setHoverEvent(
                                                new HoverEvent(HoverEvent.Action.SHOW_TEXT,
                                                        new ComponentBuilder(ChatColor.translateAlternateColorCodes('&',
                                                                getConfig().getString("Messages." + toSay + ".Hover")))
                                                                        .create()));
                                    else if (!getConfig().getString("Messages." + toSay + ".Click").isEmpty()) {
                                            if ("COMMAND".equals(type))
                                                text.setClickEvent(new ClickEvent(ClickEvent.Action.RUN_COMMAND,
                                                        "/" + getConfig().getString("Messages." + toSay + ".Click")));
                                            else if ("LINK".equals(type))
                                                text.setClickEvent(new ClickEvent(ClickEvent.Action.OPEN_URL,
                                                        main.getConfig().getString("Messages." + toSay + ".Click")));
                                    }
                                    online.spigot().sendMessage(text);
                                }
                                try {
                                    online.playSound(online.getLocation(),
                                            Sound.valueOf(getConfig().getString("Messages." + toSay + ".Sound")), 1f,
                                            1f);
                                    online.spawnParticle(Particle.valueOf(getConfig().getString("Messages." + toSay + ".Particle")), online.getLocation(), 25);
                                } catch (IllegalArgumentException | NullPointerException exc) {
                                    exc.printStackTrace();
                                }
                                for (String cmdasp : getConfig().getStringList("Messages." + toSay + ".CommandsAsPlayer"))
                                    online.performCommand(cmdasp);
                                for (String cmdasc : getConfig().getStringList("Messages." + toSay + "CommandsAsConsole"))
                                    Bukkit.dispatchCommand(Bukkit.getConsoleSender(),
                                            cmdasc.replace("%player%", online.getName()));
                            }
                        //}).start();
                }
            }.runTaskTimer(this, 0, /*don't know if this cast is necessary, but just to be sure*/(long) getConfig().getInt("Delay") * 20);
        }
    }
     
    This is all I could find for now, I'll let you know if I find more stuff.
     
    #15 Stef, Jun 6, 2017
    Last edited: Jun 6, 2017
    • Agree Agree x 2
    • Winner Winner x 1
    • Informative Informative x 1
  16. This would usually require shading Commons, if you don't already have it there's nothing wrong with just catching an exception.
     

  17. Thanks! I will change everything u said tomorrow. The thread is to make everything aSync.
     
  18. Wow you took alot of effort to write that, nice work, surprised you didnt suggest at least putting the config strings into a cached field for readability though lol
     
  19. And this is shaded by Mojang into Minecraft pretty sure, either that or by Spigot. If it's there, and youre using the jar that shades it, why not use it? Also, if this isn't provided, and for whatever reason you're on a Spigot forum without intent to use Spigot, then you could use regex. No need for try catch there.
     
  20. If this code works when it's run async (many bukkit stuff doesn't so check that), you can change the runTaskTimer to runTaskTimerAsynchronously. Saves typing.