Is my coding messy?

Hello, I am a beginner at Java, and I have heard that the tidiness of your code really matters. I cannot really tell if my code is tidy or messy, so I would like feedback from others. Here is my code:

Code:

public class Speed implements CommandExecutor {

    public boolean onCommand(CommandSender sender, Command cmd, String commandLabel, String[] args) {

        Player p = (Player) sender;

        if (cmd.getName().equalsIgnoreCase("speed")) {

            if (p.hasPermission("custom_speed") || p.isOp()) {

                if (args.length < 1) {
                    p.sendMessage(ChatColor.RED + "Usage: /speed (speed)");
                    return true;
                }

                float n = Float.parseFloat(args[0]);

                if (n > 10 || n < 0) {
                    p.sendMessage(ChatColor.GRAY + "Please specify a " + ChatColor.GOLD + "speed" + ChatColor.GRAY + " from " + ChatColor.GOLD + "0 - 10" + ChatColor.GRAY + "!");
                    return true;
                }

                if (!(p.getLocation().getBlock().getRelative(BlockFace.DOWN).getType() == Material.AIR)) {
                    p.setWalkSpeed(n / 10);
                    p.sendMessage(ChatColor.GRAY + "Your " + ChatColor.GOLD + "walk speed" + ChatColor.GRAY + " has been set to " + ChatColor.GOLD + n + ChatColor.GRAY + "!");
                    return true;
                }

                p.setFlySpeed(n / 10);
                p.sendMessage(ChatColor.GRAY + "Your " + ChatColor.GOLD + "fly speed" + ChatColor.GRAY + " has been set to " + ChatColor.GOLD + n + ChatColor.GRAY + "!");
                return true;

            } else {
                p.sendMessage(ChatColor.RED + "You do not have permission to use /speed!");
            }
        }
        return true;
    }
}

 

@Graffetus
IMO it looks fine That's how I would do it

 

The code looks fine, personally I would avoid extremely long lines, if you're using a lot of ChatColors then you can statically import all of them and just refer to them as RED, BLUE, YELLOW and so on.

Whatever format you choose for indentation and spacing shouldn't matter as long as you're consistent with it

@Graffetus

 

@Graffetus

1. Do not blindly cast object! You are bound to get errors that way. Check if the sender is an instanceof player before casting sender to a player
2. The reason why we have the ability to return false is so that you can get the "useage" message from the plugin.yml. There is no need to return true and print out the usage when you can just return false and use the commad's usage from the plugin.yml
3. Check if player.isOnGround() instead of getting block relative.
4. Only return true when the command worked as it should.

Although you seem to be on the right track, you Must learn Java before working on bukkit. Not after. Not while. Because you have to write plugins in Java, you should fully understand Java in order to prevent you from making simple mistakes and allow you to understand how to fix whatever problems you come across: Please take a look at the following link, find a resource, take some time to learn Java, and come back:
https://bukkit.org/threads/plugin-dev-sticky-learning-java-where-to-learn.395662/

 

@Graffetus Also, you never checked to make sure args[0] is a float. You should surround it with a try catch.


@Zombie_Striker Are you able to use colors with the plugin.yml usage? I've never looked into it.

 

@Xerox262
Yes. You can use "&" and use ChatColor.translateAlternateColorCodes() to turn "&" into paragraph symbols.

 

@Zombie_Striker Actually, not with plugin.yml usage.
@Xerox262 Put quotes around it, and then use \xa7 as your color code
@Graffetus What I usually do is put this method somewhere:

Code:

String c(String s) {
    return ChatColor.translateAlternateColorCodes('&', s);
}

Then I just put c() around my messages, and use & color codes.

 

My previous speakers told you some important things. But i also have some suggestions which might help you, i commented the points at where i changed something:

Code:

public class Speed implements CommandExecutor {
    public boolean onCommand(CommandSender sender, Command cmd, String commandLabel, String[] args) {
        if (!(sender instanceof Player)) {
        sender.sendMessage("No no, Players only please.");
        return true;
        }
       
        Player p = (Player) sender;

        // you dont need to check if it matches "speed" if this is the only command in the class
       
        //you can invert checks like this to prevent unnecessary nesting
        //(so not your whole code is surrounded by it, if you do this 2-3 times it becomes very messy)
            if (!p.hasPermission("custom_speed") && !p.isOp()) {
                p.sendMessage(ChatColor.RED + "You do not have permission to use /speed!");
                return true;
            }
                if (args.length < 1) {
                    p.sendMessage(ChatColor.RED + "Usage: /speed (speed)");
                    return true;
                }
                float n = Float.parseFloat(args[0]);
                if (n > 10 || n < 0) {
                    p.sendMessage(ChatColor.GRAY + "Please specify a " + ChatColor.GOLD + "speed" + ChatColor.GRAY + " from " + ChatColor.GOLD + "0 - 10" + ChatColor.GRAY + "!");
                    return true;
                }
                if (!(p.getLocation().getBlock().getRelative(BlockFace.DOWN).getType() == Material.AIR)) {
                    p.setWalkSpeed(n / 10);
                    p.sendMessage(ChatColor.GRAY + "Your " + ChatColor.GOLD + "walk speed" + ChatColor.GRAY + " has been set to " + ChatColor.GOLD + n + ChatColor.GRAY + "!");
                    return true;
                }
                p.setFlySpeed(n / 10);
                p.sendMessage(ChatColor.GRAY + "Your " + ChatColor.GOLD + "fly speed" + ChatColor.GRAY + " has been set to " + ChatColor.GOLD + n + ChatColor.GRAY + "!");
                return true;
    }
}

Another little thing, this is just matter of taste:

Code:

// if you have checks with only one statement after it, you dont need the brackets:

if (isItTrue()) doSomething();
else doSomeThingElse();

//or

if (isItTrue())
   doSomeThing();
else
   doSomeThingElse();

 

Yeah for stuff like commands. And when i do like lobg messages to players i can just call a method when i want ro do it and stuff

 

@pie_flavor Or you can

Code:

CommandManager executor = new CommandManager(this);
for(String commandName:getDescription().getCommands().keySet())
{ PluginCommand command = getCommand(commandName);
  command.setExecutor(executor);
  command.setUsage(ChatColor.translateAlternateColorCodes('&', command.getUsage()));
  command.setPermissionMessage(ChatColor.translateAlternateColorCodes('&', command.getPermissionMessage()));
}

and then it will translate the color codes in the plugin yml

 

Thats not really on topic, we´re talking about mr. @Graffetus code.