Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am working on a plugin for a Minecraft server. In it, I have a function that returns a ChatColor depending on what permission level you are.

Whenever I need reference the user with proper colors, I always get a color from the below function.

/**
 * ChatColor
 *
 * @param Player
 * @return ChatColor
 */
public ChatColor getColor(Player p) {
    // By default the color is Guest. The color will stay at guest otherwise.
    ChatColor color = ChatColor.GRAY;
    if (p.hasPermission("level.owner")) {
        // Owner
        color = ChatColor.RED;
    } else if (p.hasPermission("level.staff")) {
        // Admin / Mod
        color = ChatColor.AQUA;
    } else if (p.hasPermission("level.builder")) {
        // Builder
        color = ChatColor.DARK_AQUA;
    } else if (p.hasPermission("level.member")) {
        // Member
        color = ChatColor.GOLD;
    } else if (p.hasPermission("level.guest")) {
        // Guest
        // We don't need to change the color, since the color defaults to gray.
    }
    return color;
}

One might call this function as below:

p.sendMessage("Welcome back, " + getColor(p) + p.getName() + gold + "!");

And it would send a message to the player like "Welcome back, (playerName)!" with playerName the level's color.

What I have found is that if you have the first permission level (level.owner) it can take a much longer time. After putting in some debugging (e.g getting a NanoTime, running my function, and subtracting the difference) I've found the following:

  • With level.owner it takes about 4580ns to run
  • With level.staff it takes about 6340ns to run
  • With level.builder it takes about 8000ns to run
  • With level.member it takes about 7630ns (sometimes going as high as 10240ns) to run
  • With level.guest it takes about 10000ns+ to run

Is there any way to improve my function run at nearly the same speed for all of my checks?

(I am a beginner at Java, but know many other languages, so this is somewhat new to me.)

share|improve this question
1  
Sounds like the performance issue has to do with p.hasPermission. – EBrown Jan 20 at 0:45
up vote 7 down vote accepted

I'm guessing that you call this method frequently. If so, you might consider memoizing the result. Something like

private Map<Player, ChatColor> playerChatColors = new HashMap<>();

in the class. Possibly make this static. And then at the beginning of the method, replace

    ChatColor color = ChatColor.GRAY;

with

    ChatColor color = playerChatColors.get(p);
    if (color != null) {
        return color;
    }
    color = ChatColor.GRAY;

And then before the return at the end, add something like

    playerChatColors.put(p, color);

I don't know if this is faster or not. In the typical case, it will just do a single hash access. If that is fast, then it's faster. If that's slow, then it might not be.

There's also the problem that if the permissions change, this won't adjust. I'm assuming that this will be rare enough not to matter. If that's not so, you might need to find a way to invalidate the caching when permissions change. Of course, if you invalidate too often, you lose the benefits of caching. The first call to this method will be slower. It's the subsequent calls that this might help.

share|improve this answer
    
Thank you extremely much for your answer! While it won't completely remove the slowness of .hasPermission(), it will speed up subsequent calls. – Spotlight Jan 20 at 1:41

Nice code! It was fun to review. Note that I do not know how this API you are using works, so I've made quite a few assumptions about it. Also, I've some assumptions about your knowledge of Java (despite your post) because you said you were familiar with other programming languages, so if you have any questions you can ask me in a comment.


Different times

The reason why you have different times for each level is because of these else ifs. Since this is a chain of else ifs, when one passes, the entire rest of the chain is skipped.

That means, the player is level.owner, as soon as that first check passes, the rest of the checks are completely skipped and the color is returned. However, if the user is a level.guest, the execution has to go through the entire chain before checking for level.guest.

A way you could possibly even these out would be to remove the else part of each conditional. That way, each conditional has to be at least checked before the return statement is reached.

However, I don't see why you would want to do that because if it's possible to keep your function fast like it is now, then there is no point in trying to make it slower so the speeds are even.

In fact, you could make this function even a bit faster by returning the color as soon as the correct conditional is met.


Converting to switch

Now, I don't know exactly how this API that you are using works, so this next part may not work.

You could try to speed up your code even more/make it more cleaner than it is right now by storing the player's permission level in the player object itself. Then, you could easily use a switch to re-write your code:

switch(p.permissionLevel) {
    case "level.owner":
        break;
    case "level.staff":
        break;
    ...
}

This permissionLevel field would most likely be set to the right level in the constructor of this player object.

Now, your code is faster because you don't have to keep calling that hasPermission method. Also, it looks much cleaner in the form of a loop.


Permission enum

Right now, you are checking what permission a player has by using this hasPermission method with these arbitrary strings. While this is okay, it could be made much more cleaner and idiomatic using an enum.

Say you had this enum:

public enum PermissionLevel {
    OWNER, STAFF, BUILDER...
}

Now, the different levels of permission are stored in one convenient place. Instead of using strings to represent the different levels, you should now use something like this.

But wait, there's more:

With this, you could also make this getColor method more efficient, along with future methods by storing the corresponding chat color with each enum. Here is what I mean:

public enum PermissionLevel {
    OWNER(ChatColor.RED), STAFF(ChatColor.AQUA)...

    ... etc methods etc...
}

Using that, you could now make this getColor method have O(1) speed:

public ChatColor getColor(Player p) {
    return p.permission.chatColor;
}

This completely eliminates the need for any conditionals and function calls, thus speeding up your code by a lot.

share|improve this answer
1  
The switch won't work. There are a set of permission levels, not a single one. See Player and click through to Permissible. – mdfst13 Jan 20 at 1:03
    
@mdfst13 You could fix that with removing the break in a few places, right? – SirPython Jan 20 at 1:07
    
No. You can't switch on a Collection in Java. You need it to return a String, but you will actually get a Set from getPermissibles. I.e. the problem is in what you call and can't be fixed from this method. – mdfst13 Jan 20 at 1:11
    
Thank you for your review! It's extremely informative. As @mdfst13 said, there is a set of permission levels. My example was with "level.staff", however the player could also have the permission "minecraft.toggledownfall", and an infinite quanity more. – Spotlight Jan 20 at 1:12
1  
@awesomebing1 Sure thing! – SirPython Jan 20 at 1:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.