Need more feedback

I already got some feedback on the code what do you guys have to say

https://github.com/JanSTuck/Simplewarps



Sent from Tapatalk

 

On your Core class @ registerCommands(); there is no need to add the command more times to shorten it.

Just add a aliases: [] section in your Plugin.yml

 

@CaptainMike2828
Also, the package names. Package names are supposed to be all lowercase letters (except for the class names, of course) and should be "<tld>.<domainyouown>.<projectname>" or "me.<yourname>.<projectname>" if you don't own a domain. So

Code:text

SimpleWarps.JanTuck.Core

should be

Code:text

me.jantuck.simplewarps.core

 

Follow Java naming conventions: Package names should be all lower case, and methods should start with a lowercase letter.

Also:

Never hide exceptions. If there is a problem, you want to be able to know what went wrong.

Although I do not like this feature, a lot of servers like delayed teleportation ("wait here for ____ seconds before teleporting"). You might want to add the Option to have delayed teleportation if you want this to be used on more servers.

 

@JanTuck

• Code:

  public void onDisable() {
  
  }

  Useless. Delete it.
• Add the @Override annotation. Makes it more expressive and clear.
• Code:

   public void RegisterPermissions() {

  As the others said, Naming conventions and this method is never called.
• Code:

  Permission warpCreate = new Permission("Simplewarps.create");
  pluginManager.addPermission(warpCreate);

  You never use that variable again, you can use addPermission(new Permission"Simplewarps.create"));, which may look nicer.
• Code:

   private SimpleWarpsCommand SWC = new SimpleWarpsCommand();

  This field is
  a) Unneeded (can be a local variable)
  b) Formatted like a constant, but not final.
  c) Not very descriptively named
• 
  
• Code:

      private World world;
      private double x, y, z;
      private float yaw, pitch;
  private String name;

  Just store the Location.
• !Comment your methods with JavaDoc!
• Code:

  public static Warp loadWarp(String name, FileConfiguration config) {

  Make "Warp" implement ConfigurationSerializable and you can deal with it like you would with a String.
• Code:

   public boolean onCommand(CommandSender commandSender, Command command, String s, String[] strings) {

  Consens is "args" for the String[] array at the end and "label" for the String "s".
• Code:

   private HashMap<String, Warp> warps = new HashMap<String, Warp>();

  Declare it as "Map" if you don't need any methods exclusive to "HashMap" (hint: you don't)
• Code:

   private HashMap<String, Warp> warps = new HashMap<String, Warp>();

  Either use a Map or the ConfigurationSection approach. I would consider making a "WarpManager" that manages the warps, addition, deletion and saving/loading. This way you can eleminate the ConfigurationSection and the map from the CommandListener. They don't belong there, if you think of it logically. Why would you put your data in the CommandListener? What happens if you later want to access the data from a different class?
• Code:

   private File warpStorage = new File("plugins/SimpleWarps/warps.yml");

  This could also be done in the WarpManager, if you chose to create one
• Code:

   Warp loadWarp = Warp.loadWarp(teleportWarpName, configuration);

  Creating a new Warp object from the Representation in the ConfigSection is not the best idea, unless it implements ConfigurationSerializable. In the latter case it is a simple Map lookup, in the former it needs to be assembled correcty, which just wastes time. There is just no need to do it this way, if it can be solved easier AND more efficient with implementing that interface. You cache it in the "warps" map, but if you used the interface way (or/and a Manager) it would be cached automatically. It is also so small, that even having hundreds of warps loaded won't impact the server.

There is certainly more, but this was already nitpicking at times. I am fully aware that my code may not be better, but I think the things I mentioned are valid points. If you disagree (you, @Zombie_Striker for example), feel free to do so, but please provide a small explanation why you did.

Thanks
Hope you all have a nice day!

 

Names are not important, especially since he never references "s".

In case @CaptainMike2828 never worked with ConfiguationSerializable:
http://wiki.bukkit.org/Configuration_API_Reference#Serializing_and_Deserializing_Objects

I agree with this, and I also want to add onto this. If a world has not been loaded/loaded correctly, the world object may be null. Since plugins like MultiWorld load worlds only when the plugin has been enabled, if for some reason this plugin loads before MW, then the world may be null. Make sure the world is not null before creating the Location object.

No, these permissions should be fields, and the command should reference these objects when they check for permissions. Make it easier to manage if he ever wants to change the permission names.

 

The onCommand function was autogenerated by intelliJ

Sent from Tapatalk