Code efficiency

Discussion in 'Plugin Development' started by Louis1234567890987654321, Jan 14, 2014.

?

Was this tutorial good? :)

  1. Yes

    0 vote(s)
    0.0%
  2. No

    3 vote(s)
    50.0%
  3. I don't know

    3 vote(s)
    50.0%
Thread Status:
Not open for further replies.
  1. It has come to my concern, many developers claim that their code lag the server too much.

    For example, I am trying to create a plugin that tps all players to an operator (OP) when any player dies. Consider the code below:

    Code:
    @EventHandler
    public void onPlayerDie(PlayerDeathEvent e){ //player death event
        for(int j=0;j<Bukkit.getOnlinePlayers().length;j++){ //loops through all online players
            if(Bukkit.getOnlinePlayers()[j]!=getOperator()){ //if the online player is not the operator
                if(getOperator()==null){
                    continue;
                }
            Bukkit.getOnlinePlayers()[j].teleport(getOperator().getLocation()); //teleports
            }
        }
    }
     
    public Player getOperator(){
        for(int j=0;j<Bukkit.getOnlinePlayers().length;j++){ //loops through all online players
            if(Bukkit.getOnlinePlayers()[j].isOp()){ //checks if the player has operator status
                return Bukkit.getOnlinePlayers()[j]; //returns the operator
            }
        }
        return null; //returns null if not found
    }
    
    What is wrong with the code above?

    Yes, the code works, but the code is VERY inefficient. But why?

    Lets look at the method getOperator();

    Code:
    public Player getOperator(){
        for(int j=0;j<Bukkit.getOnlinePlayers().length;j++){ //loops through all online players
            if(Bukkit.getOnlinePlayers()[j].isOp()){ //this makes Bukkit create a new array in every loop!
                return Bukkit.getOnlinePlayers()[j]; //wait, theres two arrays!
            }
        }
        return null; //returns null if not found
    }
    
    As you can see, the above method creates SO MANY arrays, which grabs ram and lags the server.

    Lets look at the method onPlayerDie(PlayerDeathEvent)

    Code:
    @EventHandler //event handler
    public void onPlayerDie(PlayerDeathEvent e){ //player death event
        for(int j=0;j<Bukkit.getOnlinePlayers().length;j++){ //loops through all online players
            if(Bukkit.getOnlinePlayers()!=getOperator()){ //similarly, this creates a new array every loop! moreover, this calls getOperator() in every loop, which getOperator has a loop itself!
                if(getOperator()==null){ //this calls getOperator again!
                    continue;
                }
    Bukkit.getOnlinePlayers().teleport(getOperator().getLocation()); //creates another array and calls getOperator again!
            }
        }
    }




    This proves that the code is very inefficient. If X is the number of online players, the code checks Bukkit.getOnlinePlayers.isOp 3X^2 times in worst case. (Three loops in a loop)



    How could we make this more efficient?
    To make this code more efficient, simply reduce loops and arrays!



    Code:
     
     
    @EventHandler
    public void onPlayerDie(PlayerDeathEvent e){
        Player operator=getOperator(); //still loops through all online players, but this is called only once :)
        if(operator==null){
            return; //returns is no operator is online
        }
        Player [] players = Bukkit.getOnlinePlayers(); //allocates 1 array
        for(int j=0;j<players.length;j++){ //loops through all online players
            if(players[j]!=operator){ //if the online player is not the operator
                players[j].teleport(operator.getLocation()); //teleports
            }
        }
    }
     
    public Player getOperator(){
        Player [] players = Bukkit.getOnlinePlayers(); //allocates another array
        for(int j=0;j<players.length;j++){ //loops through all online players
            if(players[j].isOp()){ //checks if the player has operator status
                return players; //returns the operator
            }
        }
        return null; //returns null if not found
    }
    
    I admit that the code above is not the most efficient, however it MUCH MORE efficient than the last one. If X is the number of online players, the code checks players.isOp() 2X times in worst case. (2 loops)

    This is my first tutorial :), any comments are appreciated!

    why are there [/I] and s in the code...

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 6, 2016
  2. Offline

    Ch4t4r

    Wrong section I guess.
     
  3. Which section should I post it in?
     
  4. Offline

    Ch4t4r

    Resources
     
  5. Sorry I can't move the thread :(
     
  6. Offline

    RawCode

    No:
    Hitting preview button before posting helps.
    There is no benchmark results, entire article is your opinion about how java works, not real information about it.

    Your code is not efficient both memory and processor time, try reading java specs before trying to write your own articles.
     
  7. How could I make it more efficient?
     
  8. Offline

    zombiekiller753

    RawCode Louis1234567890987654321
    It's not too bad. I don't see a point in creating new Player arrays, because you can just access it with the method, but I'm foggy with what is better.

    Other than that, It's much more optimized just from the movement of getOperator() from in the loop, to outside the loop. Sure, it's possible that an op joins during the loop, but those odds are slim to none.

    But, please. How does one make it more efficient? And meanwhile, let us know the parts of the JavaDocs that explain why one version is better than another
     
  9. Offline

    Garris0n

    Well it's painful to read and you should use for-each loops, but that's probably no more efficient.

    Not even considering that, how is an example of improving code efficiency going to help somebody? Unless you know what you're doing, you can't make it more efficient. It's as simple as that. There are an infinite number of ways to write inefficient code and optimizing some arrays isn't going to teach somebody how to fix it.
     
  10. zombiekiller753
    This is what I meant:

    Bukkit.getPlayer("a_player").sendMessage("hello");
    Bukkit.getPlayer("a_player").sendMessage("welcome");
    Bukkit.getPlayer("a_player").sendMessage("to the server");

    is less efficient than:

    Player player = Bukkit.getPlayer("a_player");
    player.sendMessage("hello");
    player.sendMessage("welcome");
    player.sendMessage("to the server");
     
  11. Offline

    zombiekiller753

    Louis1234567890987654321
    In that case, it's not because Bukkit.getPlayer also runs through the list of online players

    For-Each loops, like @Garros0n mentioned, would be the best choice here

    EDIT:
    lol. player.Bukkit.getPlayer
     
  12. Offline

    Garris0n

    Obviously because getPlayer() is relatively expensive, however if it was, say, an event, and the get method just returned a variable, it would hardly matter.

    Also, player.Bukkit?
     
  13. Offline

    RawCode

    Code:java
    1. public Player getOperator(){
    2. Player [] players = Bukkit.getOnlinePlayers(); //allocates another array
    3. for(int j=0;j<players.length;j++){ //loops through all online players
    4. if(players[j].isOp()){ //checks if the player has operator status
    5. return players; //returns the operator
    6. }
    7. }
    8. return null; //returns null if not found
    9. }


    0) You code return array (but must return player), what about testing your code before posting article, this is not plugin that noone will ever download.
    1) In most cases operators not updated at runtime, if player is operator he will be operator "costantly".
    2) When player joined game you can check his OP state and save his name (you code dont support multiple ops)
    3) When player left you can ignore it
    3.14) Name is static variable, when needed you fetch player from stored name, if null check again, if null again - return.
    4) All other code is joke, did't read
     
  14. @RawCode

    the code is just an example, not expecting users to download it or post it @ DBO. As for the return part, I replaced and without adding [j] back , sorry for that. The code meant to teleport all players to just one operator, so we don't need multiple operators. Because this is an example, I didn't want to add more events to it (like player join event) because I have to type more.

    Anyway, thanks for your time :)
     
  15. Offline

    desht

    That is not a safe assumption. Any number of security holes can be traced back to that sort of flawed optimisation.


    You're being a little bit offensive here. There are a couple of flaws in the code (and this thread should be in Resources forum, or possibly in the Common Mistakes thread), but it's not a joke. There is a good point here, which is that Bukkit.getOnlinePlayers() allocates a new array every time it's called. For a busy server with many players, that overhead adds up so it makes sense to get a copy of the list once and work on that.
     
    Garris0n likes this.
  16. desht

    Sorry I couldn't move the thread.
     
  17. Offline

    Garris0n

    While that is true and would make sense to post, this post seems to be more about "teaching how to code efficiently", which doesn't make any sense. Showing somebody how to optimize one thing won't teach them to code efficiently, understanding of the code will.
     
    desht likes this.
  18. Offline

    desht

    You can hit the Report button and ask for a forum mod to move it into Resources.
     
    Garris0n likes this.
  19. @Garris0n

    So do you have any comments that I could use it to improve the tutorial?
     
  20. Offline

    Garris0n

    There just isn't a way to write a tutorial on it. The tutorial is http://docs.oracle.com/javase/tutorial/ , put simply.
     
  21. What you mean is the tutorial I posted is useless...

    Thanks for the comment anyway.
     
  22. Offline

    Garris0n

    It's not totally useless, as desht pointed out it does tell people that they shouldn't continue to use Bukkit.getOnlinePlayers() as it constructs a new array every time. The thing is, that belongs more in the Common Mistakes thread than in its own post.
     
  23. Offline

    RawCode

    Code:java
    1. @SuppressWarnings("unchecked")
    2. public Player[] getOnlinePlayers() {
    3. List<EntityPlayer> online = playerList.players;
    4. Player[] players = new Player[online.size()];
    5.  
    6. for (int i = 0; i < players.length; i++) {
    7. players[i] = online.get(i).playerConnection.getPlayer();
    8. }
    9.  
    10. return players;
    11. }[/i]


    anyone with a bit of "skill" can hit f3 and get source code of method, there is absolutely no reason to post article about such thing.

    desht

    It's ok for code to have flaws in plugins (that noone will download) , but such severe issues inside article about "how to code properly" looks like trolling for me.
     
  24. RawCode i typed the code in this website.

    control+click works to get the source code of the method too
     
  25. Offline

    Garris0n

    What about f3?
     
  26. he said f3?
     
  27. Offline

    Garris0n

    But what does he mean by "you can hit f3"?
     
  28. Offline

    RawCode

    pressing F3 shows source code of method in eclipse...

    this is very very very sad.
     
  29. Offline

    Garris0n

    I don't use eclipse.
     
Thread Status:
Not open for further replies.

Share This Page