I want your opinion about my code readability

Discussion in 'Spigot Plugin Development' started by FrozenLegend, Oct 13, 2018.

  1. Hi everyone,

    I'm making a "resource" for easily create interactive GUI and i was wondering if my current way to do it has a good readability or if you think i should already change something.
    (method name, class name, better way to do something etc...)

    Current working code

    Code (Java):
    final GUIManager guiManager = new GUIManager(plugin);

    final GUI gui = guiManager.create("teleport_gui", player);

    final Page page = new Page("Teleports", Size.THREE_ROW);

    final Pattern pattern = page.createPattern();
    final Map<Character, ItemStack> items = new HashMap<>();

    pattern.set(Row.FIRST,  "RBBBWBBBR");
    pattern.set(Row.SECOND, "B       B");
    pattern.set(Row.THIRD,  "RBBBWBBBR");

    items.put('B', new ItemStack(Material.BLACK_STAINED_GLASS_PANE));
    items.put('R', new ItemStack(Material.RED_STAINED_GLASS_PANE));
    items.put('W', new ItemStack(Material.WHITE_STAINED_GLASS_PANE));

    page.applyPattern(pattern, items);

    page.addItem(Row.SECOND, Column.FIFTH, new ActionItem(new ItemStack(Material.COMPASS)) {
        @Override
        public void onClick(Player player, ClickType clickType) {
            player.teleport(player.getLocation().getWorld().getSpawnLocation());
        }
    });

    gui.addPage(page);
    gui.open();

    Result:
    View attachment 377976
    The player is teleported when clicking on the compass
     
    #1 FrozenLegend, Oct 13, 2018
    Last edited: Oct 13, 2018
  2. Honestly, this is very readable. :clap: :clap: only thing i would change is

    Code (Java):
    @Override
    public void click(Player player) {
        main.show(menu);
    }
    to

    Code (Java):
    @Override
    public void onClick(Player player) {
        main.show(menu);
    }
    Also, the onClick method should also accept the ClickType so different actions can be done based on Right clicking, leftclicking.
    A boolean can also be added to the method. This boolean would be true if the player is shift clicking and false if not.
     
    • Agree Agree x 1
  3. Thanks for the reply :)
    Yes renaming to onClick and giving access to ClickType seems really cool but for the boolean you can already get it using ClickType#isShiftClick() !
     
  4. I would remove the final modifiers as it's mostly redundant and if you're using Java 10 and above, replace the explicit types with var. Also if ActionItem contains only a single method and is mostly stateless I would convert it to a lambda expression instead,
     
  5. I don't like using var when the output of something is not clear like here
    Code (Java):
    var main = GUIManager.create("main", player, plugin);
    so
    Code (Java):
    GUI main = GUIManager.create("main", player, plugin);
    is much more readable but why not for the pages because we know exactly what they are
    Code (Java):
    var menu = new Page("Menu", Size.ONE_ROW);
    and i'm using final keyword because : it's final, so i won't remove them
     
    • Agree Agree x 1
  6. I'd also add another method to add an item at an (x, y) location - I'd assume anyone using this system may want to add items in some sort of loop, and this would be a little difficult to do using an enum. I'd do this for all methods which act on a position in your GUI really
     
  7. It's already available you can also add an item using inventory slot id

    I've also added
    Code (Java):
    page.addBorder(new ItemStack(Material.BLACK_STAINED_GLASS_PANE), Border.TOP, Border.BOTTOM);
    who give something like this with a page using Size.THREE_ROW
    upload_2018-10-13_20-18-4.png
    You can add border to all side of the page (top, bottom, left, right)
     
  8. Maybe, you can add borders the way crafting recipes are made. It would allow for very configurable borders or designs of the gui
     
  9. Something like this ?

    Code (Java):
    final Page page = new Page("MyPage", Size.THREE_ROW);

    final Pattern pattern = page.createPattern();
    final Map<Character, ItemStack> items = new HashMap<>();

    pattern.set(Row.FIRST,  "RBBBWBBBR");
    pattern.set(Row.SECOND, "B       B");
    pattern.set(Row.FOURTH, "RBBBWBBBR");

    items.put('B', new ItemStack(Material.BLACK_STAINED_GLASS_PANE));
    items.put('R', new ItemStack(Material.RED_STAINED_GLASS_PANE));
    items.put('W', new ItemStack(Material.WHITE_STAINED_GLASS_PANE));

    page.applyPattern(pattern, items);
    Result :
    upload_2018-10-13_21-6-9.png
     
    #9 FrozenLegend, Oct 13, 2018
    Last edited: Oct 13, 2018
  10. When you use the chars for your pattern, is it important to keep track of the spaces you've made in row 2 and 3? If so maybe there should be a char representing space? Maybe that will prevent some errors.
     
  11. Empty char in the string = empty slot in the inventory

    Edit : Current working code

    Code (Java):
    final GUIManager guiManager = new GUIManager(plugin);

    final GUI gui = guiManager.create("teleport_gui", player);

    final Page page = new Page("Teleports", Size.THREE_ROW);

    final Pattern pattern = page.createPattern();
    final Map<Character, ItemStack> items = new HashMap<>();

    pattern.set(Row.FIRST,  "RBBBWBBBR");
    pattern.set(Row.SECOND, "B       B");
    pattern.set(Row.THIRD,  "RBBBWBBBR");

    items.put('B', new ItemStack(Material.BLACK_STAINED_GLASS_PANE));
    items.put('R', new ItemStack(Material.RED_STAINED_GLASS_PANE));
    items.put('W', new ItemStack(Material.WHITE_STAINED_GLASS_PANE));

    page.applyPattern(pattern, items);

    page.addItem(Row.SECOND, Column.FIFTH, new ActionItem(new ItemStack(Material.COMPASS)) {
        @Override
        public void onClick(Player player, ClickType clickType) {
            player.teleport(player.getLocation().getWorld().getSpawnLocation());
        }
    });

    gui.addPage(page);
    gui.open();

    Result:
    upload_2018-10-13_22-52-0.png
    The player is teleported when clicking on the compass
     
    #11 FrozenLegend, Oct 13, 2018
    Last edited: Oct 13, 2018
  12. Yes!! Now I want to use this :D
     
  13. Lots of spaces in code etc! Very nice
     
  14. The code is easily readable, nicely split up and nothing really wrong with it. Naming conventions are followed nicely as well. Then again this is only a very small blob of code. The real fun kicks in when making much larger applications, where you have to keep things readable through actual code principles & architecture ;)
     

Share This Page