Code Cleanup - Custom Enchants

Discussion in 'Spigot Plugin Development' started by MCGamer00000, Jul 10, 2018.

  1. I figured I could use a bit of help cleaning my code up.
    It works great, but I figured I'd ask if it could be improved.

    https://github.com/MCGamer00000/CustomEnchants/

    If you have any questions about it, or possible changes to make, you can reply, DM me, and/or make a pull request or issue.
     
    #1 MCGamer00000, Jul 10, 2018
    Last edited: Jul 14, 2018
  2. I think you should use:
    Code (Java):
    e.getPlayer().getInventory().getItemInMainHand();
    instead of:
    Code (Java):
    e.getPlayer().getItemInHand();
    if you are programming for 1.9+ because there are two hands. But i'm not so sure
     
  3. Oh yeah, forgot about that, I didn't know how to do it in 1.9+ when I wrote it, then I figured it out and forgot to change it.
    Thanks
     
    • Like Like x 1
  4. _Ug

    _Ug

    For getting an enchant, this would be far more efficient:

    Code (Java):
            //Name of the enchantment in the lore (without color)
            String enchant = "Explosive Pickaxe 2";
            //List of each word in the string
            String[] enchantAsList = enchant.split(" ");
            //The last word of the string, where the level is
            String levelString = enchantAsList[enchantAsList.length - 1];
            //If the level is not composed of digits, return
            if(!levelString.matches("\\d+")) {
                return;
            }
            //Finally you are left with enchantName, in this case "Explosive Pickaxe" and the integer level, in this case 2.
            String enchantName = enchant.replaceAll(" " + levelString, "");
            int level = Integer.parseInt(levelString);
    Edit: In your case, you would not return but continue as you are looping through the lore.
     
    • Winner Winner x 1
  5. I would like to stick to Minecraft's default enchantments as much as possible, so how would I convert roman numerals to an integer efficiently?
    But, I do completely understand the over complication of my code.
     
  6. Choco

    Moderator

    CustomEnchants#L34-L35: Sooooo... if (args.length == 1) ?
    CustomEnchants#L36: Why not just make this check at the beginning of your command. You do it on L47 anyways. Remove both checks and do it at the top instead.
    CustomEnchants#L51-L57: #getMaterial() does not throw an exception, it returns null if invalid. That being said, I recommend #matchMaterial() as it doesn't care about case. Also worth noting that catching "Exception" is incredibly poor practice, especially when an exception isn't explicitly thrown.
    CustomEnchants#L58-L61: Use a StringBuilder instead. Every time you concatenate in a loop, it creates a new StringBuilder instance which is a lot of unnecessary object creations. 1 builder, append in the loop.
    CustomEnchants#L63: Good for you for using COLOR_CHAR over the section symbol, but see ChatColor#translateAlternateColorCodes()
    CustomEnchants#L81-L83: I'm not sure that this needs a setter. It's a manager for your plugin, why would you want anyone to set it and potentially break your entire plugin?

    BlockBreakEnchant: Consider instead an interface. No reason to be an abstract class here. As an added bonus, it may be annotated as @FunctionalInterface

    Enchant#L5-L6: private and final fields

    EnchantManager#L14: I recommend that the key be a NamespacedKey for the sake of name clashing. If I wanted to register my own "explosion" enchantment, it would override yours. Using NamespacedKey lets you separate enchantments by associating them to their plugin to avoid clashing.
    EnchantManager#L21-L22: Looks kind of sloppy. Create a List once, then check null, return the list. Continue to use that same list if not null.
    EnchantManager#L27-L28: These are constants, declare them as such. private static final fields
    EnchantManager#L48-L50: Again, ChatColor has a method for this
    EnchantManager#L52-L54: The returned value should either be a clone or immutable. I should not be able to modify this Map. Create methods to register / unregister enchantments that modify the Map instead. Otherwise, this looks incredibly sloppy and poses issues.

    Overall, this class really doesn't look all that sturdy. Relying on lore than may be modified by another plugin at any point in time is kind of risky and awkward. Consider either NBT or extending the existing Enchantment API

    ExplosionEnchant#L18: Noooo, no no no. Do not create a new instance of Random every time you need it. These are expensive. Create a constant or use ThreadLocalRandom#current();

    That was the most I was willing to pick out, but I think you get the point. There are better ways to handle a lot of what you're doing, but I appreciate the effort. Always seek for improvements or you'll never learn, so I'm glad you made this thread. Most are too afraid to accept criticism. Take mine with a grain of salt; these are just things I picked out and need not be religiously followed, but they're my suggestions based on the current state of your code. Do not ignore anyone else's criticism if given in this thread; all opinions are worth listening to, and if they're incorrect, someone will politely indicate otherwise.

    I'd also like to just throw this out there: Good for you for using GitHub and making your code open source. It's rare that we see open sourced code on here, but I'd take my reply as proof that open source helps other developers improve your overall code quality.
     
    • Like Like x 1
    • Winner Winner x 1
    • Friendly Friendly x 1
  7. This is the reason I made this thread, I knew there would be people like you that would go through every single detail and tell me whats could be done differently.
    Although there are some things in there that wouldn't be needed, this list was very helpful.

    I am extremely tempted to explain most of this stuff, but I know you don't want to hear that, so I'll just edit accordingly.

    Thanks for all the suggestions, I will go through them, and look into each one.
     
    #7 MCGamer00000, Jul 10, 2018
    Last edited: Jul 10, 2018
  8. Two -small- things that I would probably change:

    EnchantManager.java#L21 : Might want to return Collections.emptyList() instead to cut down on unnecessary object creation. Not that it really matters here, but probably good practice to get used to.

    BlockBreakListener.java#L25 : Could return early (before the loop) if the enchantments of the item are empty, because there'd be no point in looping through all blocks anyway.
     
  9. Choco

    Moderator

    Worth noting that Collections#emptyList() creates a new, immutable List implementation. I considered noting that in my reply as well, but figured I'd let it slide because I instead mentioned the "create once, reuse if necessary" pattern.
     
  10. I just did what 2008Choco said.
    Yeah definitely, that will improve the performance significantly, I'll add that in the newest commit.
    Thanks for the reply.
     
  11. Just what does ChatColor.stripColor do?
    I understand that it removes the color off a string, but how?
    Lets say I have the color coded version of "&7Hey"
    If I stripped it, would it be "7Hey"
    or would it be "Hey".

    I haven't used it before, because I keep finding seemingly conflicting uses for it from other people.
    The javadocs don't help also because it just says it removes the colors.

    (Also, just to say, the reason I didn't use it before was that I thought it didn't do everything I wanted it to do)
     
  12. Choco

    Moderator

    It uses regular expression to search for and replace all instances of COLOR_CHAR[1-9A-FK-OR] (so §1, §2, §a, so on and so forth) with an empty String. Basically, it removes all colour codes from the String entirely.

    "&7Hey" -> translated -> "§7Hey" -> stripped -> "Hey"
     
  13. Ok thanks for clearing that up.
     
  14. Choco

    Moderator

    • Friendly Friendly x 1
  15. Not really related to the thread, I'm sorry, but just to clear up, Collections#emptyList() does not create a new object, no, it returns an already-existing instance, which is why it's a good way to avoid unnecessary Object creation. Not sure if it's just me confused by Choco's sentence, but just in case.
    [​IMG]
     
  16. While expanding your scope by a bit here are my two pennies on this:

    How about storing the enchantments proper under nbt data & using a packet adapter on the mirror items packet to write it to your lore? Like that you would also have a clean option to rename your enchantments if required & won't clutter the lore when your plugin is removed.
     
    • Agree Agree x 1
  17. While I know that would be more proper and efficient in the long run, right now I would rather keep it to code that I understand, and that I know how to use it.
    Not to mention the NMS needed if I want my plugin standalone.

    Thanks for the thought, but I'll have to decline.
     
  18. Bump

    While it may seem I have completed what I wanted, I have updated it further with a lot of new code.