[Soup] The fast way!

There are various ways I have seen people making insta-soup plugins, but I though I should make a thread to set the record straight, on what the fastest and most efficient way to add soup, and the actual mechanic of doing so!

Adding Soup (the efficient way)
​

Code:

ItemStack stew = new ItemStack(Material.MUSHROOM_STEW, 1); //Creating the ItemStack

            int soup = 35; //Making a new Integer
            while (soup >= 1) { //When soup is greater than 1
                p.getInventory().addItem(stew); //We are now adding the ItemStack we made before
                soup--;

Soup Mechanic (the awesome one we all want)​

Code:

    @EventHandler(priority = EventPriority.HIGHEST)

       public void FastSoup(PlayerInteractEvent e){
        Player p = e.getPlayer(); //Getting the instance of the player
        int hp = 7; //Adding a new Integer 7 (3.5 hearts healing)
        double h = ((Damageable)p).getHealth();
        Action a = e.getAction();
        if(h != ((Damageable) p).getMaxHealth()){
            if(h != 0){ //If there health is NOT at zero, preform what is below.
                if ((a == Action.RIGHT_CLICK_AIR) || (a == Action.RIGHT_CLICK_BLOCK)){ //Checking if they rightclicked air or (||) Rich clicked a block
                    if (p.getItemInHand().getType() == Material.RABBIT_STEW || p.getItemInHand().getTypeId() == 413){ //I know getTypeId is depricated, but hey, why not just have it....
                        p.setHealth(h + hp > ((Damageable)p).getMaxHealth() ?  ((Damageable)p).getMaxHealth() : h + hp);
                        p.getWorld().playSound(p.getLocation(), Sound.EAT, 3, 1); //Playing that loveley sound.
                        p.getItemInHand().setType(Material.BOWL); //Replacing that item with an empty bowl afterwards.
                        p.updateInventory(); //This is very important, you need to update their inventory, or it's like none of this ever happened!
                    }
                }
            }
        }
    }
}

 

What's your question?

 

@BrickBoy55 There isn't one. He wanted to

 

@nverdier @techkatz

In that case, wrong section.

PS: Straight*

 

Moved to Resources Section.

 

I basically wanted to set it in stone to create a good example for people to use this as reference. It's all tested and works awesome!

 

This wouldn't be good though... It would keep trying adding even when it is full. You should make a for with the amount of slots left.

 

Or you could use a for loop like a normal person...

Code:

for(int i = 0; i < 35; i++){
//Give them the item
}

Why are you casting a double to damageable?

Also you should be doing a check if their health is already 20.. if you click while they have 20 health right now it'll take soup away from them

 

@Shawckz He's not... He's casting 'p' to Damageable and then getting the health of that. Not necessary, of course.

 

ah, i misread.

 

Code:

int soup = 35; //Making a new Integer

I love this.

 

@Shawckz Why don't you just use a for loop?

 

The JVM automatically optimizes it, so it's the same as using a for loop.

 

@Phasesaber But still. Why not.

 

@nverdier For the same reason he cast Player to Damageable, he's got no idea what he's doing. He'll learn soon enough though.

 

I am just trying to make it the fastest and most efficient, a little support and encouragement would be nice. This is a resource people should be using, and I could also edit the thread. I've just started use the API and proud, so unless you have something to contribute I wouldn't talk.

 

No.

 

That's all we're doing. We're trying to help you out by showing you what you did wrong and what you can improve on.

 

This is not bukkit bash, you can be nice and not rude. There is a difference, trying to make a good resource, but it's hard when your work is criticized.

That's just mocking right there, mr.UberSuperduper developer.

 

@techkatz
You're going to have to take it. This 'criticism' is good for you; it'll definitely help you to become a better developer. You think everyone here (yes even mr.UberSuperduper developer) started off so perfect and elegant? No, of course not! They probably had people view their code when they started and 'criticised' it with the intent of helping them becoming better developers. Now, that way people are criticising you is arguable, but there is no denial that it WILL help you.

 

It is if you are using an alternative such as Spigot, because p.getMaxHealth() is deprecated for Player (so he casted it to Damageable).

Some things just don't need to be commented. The comment "//Creating a new integer" could have been changed and used on something that someone may have needed a comment on.

 

So we already had 100+ plugins doing this, and now we need a resource spoon-feeding it to new devs? Not a good idea.

 

Cleaned up Thread.

@ChipDev You had enough Threads about your deleted Resources.
Now LET IT GO

 

@techkatz It's good to be proud of what you do, but not at the point where you think it's perfect and that feedback does not matter. Gater12 said everything. Now concerning your code: keep in mind that clustered and compact code is NOT necessarily more efficient. Java is not interpreted; it's compiled. Therefore 1 letter variables and ternary operators will not increase performance. So first of all:

1) The JVM doesn't even care about how you named your variables; give them proper names:

Code:

Player player = event.getPlayer();
Action action = event.getAction();
int healthGain = 7;
double currentHealth = player.getHealth();

Now there really is no point in trying to micro-optimize this piece of code as it probably won't change anything, but if you REALLY want to set a new record:

2) You are indeed creating a new integer each time the method is invoked. This is unnecessary as the said integer will always be the same. Use a constant instead:

Code:

public static final int SOUP_HEALTH_GAIN = 7;

3) Why are you checking if the player's health is 0? How will the event fire if the player is dead? Remove the (h != 0) condition. It's also unnecessary to check whether the player is already at max health, because when adding the health gain with the current health, you are performing this check too.

4) The action should be checked first, as if the player is not right-clicking, you should stop right there.

Code:

Action action = event.getAction();
if (a != Action.RIGHT_CLICK_AIR && a != Action.RIGHT_CLICK_BLOCK) {
    return;
}
Player player = event.getPlayer();
[....]

5) player.getItemInHand() is called 3 times. Assign the value to a variable.

6) I'm quite puzzled by this line:

Code:

p.getItemInHand().getType() == Material.RABBIT_STEW || p.getItemInHand().getTypeId() == 413)

You are checking twice if the item in hand is rabbit stew, and are not checking if it's normal soup.

7) itemInHand.getType() is called twice. Assign the value to a variable.

8) In the following line, the addition is done twice:

Code:

p.setHealth(h + hp > ((Damageable)p).getMaxHealth() ?  ((Damageable)p).getMaxHealth() : h + hp);

Assign the value to a new variable and I suggest you also use Math.min():

Code:

double newHealth = currentHealth + SOUP_HEALTH_GAIN;
player.setHealth( Math.min(player.getMaxHealth(), newHealth) );

9) Updating the inventory explicitly is useless, setting the item in hand will be enough for CB to send the packet to the client.

So, if you follow all my tips, CONGRATULATIONS, your code will now take 2-3 nanoseconds less to execute! I still think the most expensive part in your method is playing the sound as CB needs to send multiple packets. But at least 3 nanoseconds will be saved .

 

I don't really care, I just wanted to help. Unlike you you meany!

There is criticism, which is bad, but there is always a sensible thing to do, which is called constructive criticism. What @Totom3 was showing me, was polite and just a great action of kindness. This is not bukkit bash, for you to show off your skills of basing people who want to learn.

 

@techkatz Your code isn't the most terrible thing in the resources section. But as always, there is room for improvement.

1. There is nothing wrong with using a while loop like you did, but there is something else that is clearly made for what you did. It's called a for-loop
3. There is no need to cast a Player to a Damageable.
4. EDIT: And method naming conventions

 

@teej107 Note: Some IDE's say casting to a Player is a "Too strong cast", and recommend, in this case, Damageable instead.

@techkatz I'm no meany :c Just not sure how it is helping

 

Here is a chart from the terms page: http://upload.wikimedia.org/wikipedia/commons/e/e1/Graham's_Hierarchy_of_Disagreement.jpg

This falls into the bottom category of the chart, while not as extreme, pointing out the problem as much before myself have done falls into Contradiction and Counterargument categories.

Additionally the Damageable thing, refer to this http://bukkit.org/threads/fragment-...-for-the-type-entityname.228249/#post-2230245