#499 - Adds banlist command #518
Conversation
|
|
| if (args.length == 1) { | ||
| return StringUtil.copyPartialMatches(args[0], BAN_TYPES, new ArrayList(BAN_TYPES_SIZE)); | ||
| } | ||
| return super.tabComplete(sender, alias, args); |
There was a problem hiding this comment.
I think this should return Collections.emptyList() if args.length is over 1 (this will remove player auto-completion, which is not necessary here)
| int index = 0, size = banEntries.size(); | ||
| final StringBuilder banList = new StringBuilder(200); | ||
|
|
||
| for (final BanEntry banEntry : banEntries) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh. Didn't see that one. Thanks
| banType = BanList.Type.NAME; | ||
| break; | ||
| default: | ||
| sender.sendMessage(ChatColor.RED + "Invalid parameter '" + parameter + "'. Usage : " + usageMessage); |
There was a problem hiding this comment.
Remove space before : ("Usage : ")
There was a problem hiding this comment.
French guy here, don't blame me.
|
|
||
| private static final int BAN_TYPES_SIZE; | ||
|
|
||
| static { |
There was a problem hiding this comment.
Is there a reason for a static initialization block here? I think you could just to:
private static final List<String> BAN_TYPES = Arrays.asList("ips", "players");No need to store BAN_TYPES_SIZE in a variable, just use BAN_TYPES.size() when needed.
There was a problem hiding this comment.
Good point, no need to store it in this case.
| sender.sendMessage("There are no banned players"); | ||
| } else { | ||
| int index = 0, size = banEntries.size(); | ||
| final StringBuilder banList = new StringBuilder(200); |
There was a problem hiding this comment.
I don't think there's a need for an initial capacity value here?
There was a problem hiding this comment.
Indeed. No need in this case.
| ++index; | ||
| } | ||
|
|
||
| sender.sendMessage("There are " + size + " banned players : "); |
There was a problem hiding this comment.
Remove space before : (" banned players : ")
There was a problem hiding this comment.
French guy here again, don't blame me.
|
|
||
| if (args.length > 0) { | ||
| final String parameter = args[0]; | ||
| switch (parameter) { |
There was a problem hiding this comment.
Make this non case-sensitive?
| default: | ||
| sender.sendMessage(ChatColor.RED + "Invalid parameter '" + parameter + "'. Usage : " + usageMessage); | ||
| return false; | ||
| if ("ips".equalsIgnoreCase(args[0])) { |
There was a problem hiding this comment.
A switch can still be used. Just make the first argument lowercase.
There was a problem hiding this comment.
Yes, less modifications would have been performed
| } else { | ||
| final List<String> targets = banEntries.stream().map(BanEntry::getTarget).collect(Collectors.toList()); | ||
| sender.sendMessage("There are " + banEntries.size() + " banned players: "); | ||
| sender.sendMessage(CommandUtils.prettyPrint(targets.toArray(new String[targets.size()]))); |
There was a problem hiding this comment.
Perhaps check if there are no banned players here so an empty message isn't sent?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yup, the check is done
There was a problem hiding this comment.
Ah, whoops, completely skimmed over it
| if (args.length == 1) { | ||
| return StringUtil.copyPartialMatches(args[0], BAN_TYPES, new ArrayList(BAN_TYPES.size())); | ||
| } else { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
You should still return super.[...] if args.length == 0 (sorry if I wasn't clear in my previous review). For example: return args.length == 0 ? super.[...] : Collections.emptyList();
There was a problem hiding this comment.
My bad. I updated it too quickly
| @@ -55,10 +55,6 @@ public boolean execute(CommandSender sender, String commandLabel, String[] args) | |||
|
|
|||
| @Override | |||
| public List<String> tabComplete(CommandSender sender, String alias, String[] args) throws IllegalArgumentException { | |||
| if (args.length == 1) { | |||
There was a problem hiding this comment.
No... you still need that check :)
if (args.length == 1) {
return StringUtil.copyPartialMatches(args[0], BAN_TYPES, new ArrayList(BAN_TYPES.size()));
}
return args.length == 0 ? super.tabComplete(sender, alias, args) : Collections.emptyList();
|
Thank you for your contribution! :) |
Hi,
This PR relates to this issues : #499. It doesn't close it, but contributes to :)
It reimplements the vanilla banlist command line.