Critique a newbies code (alternately: Tell me why I suck and how to suck less!)

So, I'm new to Java programming and I've not programmed much in ~15 years. Generally speaking my logic is functional if not efficient and I understand the basics of arrays, classes, methods, pointers, etc. That being said I've started developing my own plugin for some basic commands (because a) it's a good learning experience, b) I wanna get up to speed on bukkit plugins to help me learn java (it's always been easier for me to learn a language if I'm doing something I enjoy and am interested in with it.), and c) Most of the plugins are just way to big for what I want.

All of that being said, I have a working plugin that will summon and teleport players. What i'd like to have is some more competent programmers take a look at my code (especially) and logic (i'm not perfect) and let me know how they would write similar routines or how my code could be improved and WHY their method is better than what I'm doing (so i understand and use the methods correctly in the future rather than using them inappropriately in the wrong places) Also, since I don't really know java at all, I'm sure I'm making silly convention breaking syntactic mistakes that should be rectified for best practice reasons.

Basically critique and suggest improvements on what I'm doing, ignore the commented items that was there as a note to myself for future reference mostly.

This code does presently function without errors in the use cases I've personally tried. (existing online and offline players and non-existent players. < thanks darq! >).

( Code also available at: http://pastebin.com/n6nC3Z86 since this strips formatting. )

Code:

package net.solatium.pcommands;
 
import java.util.logging.Logger;
 
import org.bukkit.ChatColor;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
import org.bukkit.plugin.java.JavaPlugin;
 
public class PCommands extends JavaPlugin {
 
public final Logger pcLog = Logger.getLogger("Minecraft");
public static String version = "v0.1";
public String tag = "[pCommands] ";
 
@Override
public void onEnable() {
pcLog.info(tag + "is now enabled.");
}
 
@Override
public void onDisable() {
pcLog.info(tag + "is now disabled.");
}
 
@Override
public boolean onCommand(CommandSender sender, Command command, String label, String[] args) {
if (label.equalsIgnoreCase("teleport")) {
int i = 0;
String arg;
boolean pcFlagAll = false;
//boolean pcFlagWayPoint = false;
if (args.length == 0 || args.length > 2) {
//((Player) sender).sendMessage("Usage: /" + label + " [-a] [-w] <player|waypoint>");
((Player) sender).sendMessage("Usage: /" + label + " [-a] [player]");
return true;
} else {
while (i < args.length && args[i].startsWith("-")) {
arg = args[i++];
if (arg.equalsIgnoreCase("-a")) { pcFlagAll = true;}
//if (arg.equalsIgnoreCase("-w")) { pcFlagWayPoint = true; }
}
//if (pcFlagAll == true && pcFlagWayPoint == true) {
//((Player) sender).sendMessage("Sending ALL players to WAYPOINT " + args[i]);
//return true;
//} else
if (pcFlagAll == true && args.length == 1) {
((Player) sender).sendMessage("Usage: /" + label + " [-a] [player]");
return true;
} else if (pcFlagAll == true && args.length == 2) {
if(this.getServer().getPlayer(args[i]) == null) {
((Player) sender).sendMessage(ChatColor.RED + "Player not found.");
return true;
}
for (int j = 0; j < this.getServer().getOnlinePlayers().length; j++) {
this.getServer().getOnlinePlayers()[j].teleport(this.getServer().getPlayer(args[i]));
this.getServer().getOnlinePlayers()[j].sendMessage(ChatColor.GREEN + "You have been teleported to " + args[i] + " by " + ((Player) sender).getName());
}
((Player) sender).sendMessage(ChatColor.GREEN + "Sending ALL online players to " + args[i]);
//((Player) sender).sendMessage("Sending ALL players to PLAYER " + args[i]);
return true;
//} else if (pcFlagWayPoint == true) {
//((Player) sender).sendMessage("Sending SELF to WAYPOINT " + args[i]);
//return true;
} else {
if(this.getServer().getPlayer(args[i]) == null) {
((Player) sender).sendMessage(ChatColor.RED + "Player not found.");
return true;
}
((Player) sender).teleport(this.getServer().getPlayer(args[i]));
((Player) sender).sendMessage(ChatColor.GREEN + "Teleporting you to " + args[i]);
return true;
}
}
} else if (label.equalsIgnoreCase("summon")) {
int i = 0;
if (args.length == 0) {
((Player) sender).sendMessage("Usage: /" + label + " [-a] [player] [...]");
return true;
} else {
if (args[i].equalsIgnoreCase("-a")) {
for (int j = 0; j < this.getServer().getOnlinePlayers().length; j++) {
this.getServer().getOnlinePlayers()[j].teleport(((Player) sender).getLocation());
this.getServer().getOnlinePlayers()[j].sendMessage(ChatColor.GREEN + "You have been summoned by " + ((Player) sender).getName());
}
((Player) sender).sendMessage(ChatColor.GREEN + "Bringing ALL online players to you.");
return true;
} else {
for (; i < args.length; i++) {
if(this.getServer().getPlayer(args[i]) == null) {
((Player) sender).sendMessage(ChatColor.RED + "Player not found.");
return true;
}
this.getServer().getPlayer(args[i]).teleport(((Player) sender).getLocation());
((Player) sender).sendMessage(ChatColor.GREEN + "Summoning " + args[i] + " to you.");
this.getServer().getPlayer(args[i]).sendMessage(ChatColor.GREEN + "You have been summoned by " + ((Player) sender).getName());
}
return true;
 
}
}
}
return false;
}
}

Edit: Also - if you have any excellent java/bukkit tutorials you recommend I'm open to them. I went through a few but a lot didn't apply to the kind of things I was doing or alternately just flat out didn't work due to changes in java/bukkit/eclipse since they were created.

 

Ok, there're four things that could/should be changed, which would also prevent some errors that might occur.

1. Don't cast the sender to a player if you aren't sure that it's actually a player. Sure, it doesn't really make sense to use a teleport command as the console, but instead of getting a message like "You can't use this as the console" the user would get an error, which is bad. Hence, check if the sender is a player at the beginning of the command. After that you can safely cast it. However, in most cases you don't need to cast it explicitly to a player, e.g. for sending a message. The "sendMessage" method is already provided in the CommandSender interface. So you don't even need to cast the sender to something when using the sendMessage method.

2. Use command.getName() instead of label. The label can also be an alias created by the server admin in the bukkit.yml. command.getName() will always return the correct name of the command.

3. You don't really need the onEnable and onDisable if you only print out "xy is enabled" since bukkit is already doing something similar.

4. I personally prefer the plugin specific logger over the general logger, because I wouldn't need to attach my plugin tag in front of all the message. Using this.getLogger() in your onEnable would give you the plugin specific logger instance.