Solved How to improve this code?

Discussion in 'Spigot Plugin Development' started by Mysak, Jun 30, 2021.

  1. Hi I want to ask, how could I write this code more easily? I don't want to always use the if and else if conditions.

    Code (Text):

    @EventHandler
    public void onPlayerInteract(PlayerInteractEntityEvent e) {
        Player p = e.getPlayer();
        Entity clickedEntity = e.getRightClicked();

        if (clickedEntity instanceof Wolf && p.getInventory().getItemInMainHand().getType().equals(Material.STICK)) {
            if (((Tameable)clickedEntity).isTamed()) {
                if (((Tameable) clickedEntity).getOwner().equals(p)) {
                    ((Wolf) clickedEntity).setSitting(false);
                    ((Tameable) clickedEntity).setOwner(null);
                    ((Tameable) clickedEntity).setTamed(false);
                }
            }
        } else if (clickedEntity instanceof Cat && p.getInventory().getItemInMainHand().getType().equals(Material.STICK)) {
            if (((Tameable)clickedEntity).isTamed()) {
                if (((Tameable) clickedEntity).getOwner().equals(p)) {
                    ((Cat) clickedEntity).setSitting(false);
                    ((Tameable) clickedEntity).setOwner(null);
                    ((Tameable) clickedEntity).setTamed(false);
                }
            }
        } else if (etc.....)
    }
     
    Thanks.
     
  2. It depends on what you want the rest of the ifs to be, but from what I see here's what you want: when a player holds a stick in their main hand and right clicks a tameable animal (that he's the owner of) then the animal will stop sitting, become untamed, and lose its owenr (basically like releasing it back into the wild).
    If I got that right then here's what you want (replace all your ifs with this):
    Code (Java):
    if ((clickedEntity instanceof Tameable) && ((Tameable) clickedEntity).getOwner().equals(p) &&
        ((Tameable) clickedEntity).isTamed() && p.getInventory().getItemInMainHand().getType().equals(Material.STICK)) {
        if (clickedEntity instanceof Sittable) ((Sittable) clickedEntity).setSitting(false);
        ((Tameable) clickedEntity).setOwner(null);
        ((Tameable) clickedEntity).setTamed(false);
    }
     
  3. Ah that might be the solution. But now I'm getting an NPE error: Caused by: java.lang.NullPointerException: Cannot invoke "Object.equals (Object)" because the return value of "org.bukkit.entity.Tameable.getOwner ()" is null. Every time I click on an untamed animal.
     
  4. You can just check if entity is Tameable and cast

    Code (Java):

    if (p.getInventory().getItemInMainHand().getType() == Material.STICK && entity instanceof Tameable) {
      Tameable tameable = (Tameable) entity;

      if (p.equals(tameable.getOwner())) {
        if (tameable instanceof Sittable) {
          ((Sittable) tameable).setSitting(false);
        }

        tameable.setOwner(null);
        tameable.setTamed(false);
      }
    }
    EDIT: Avoid NullPointerException
     
    #4 Epicnicity2016, Jun 30, 2021
    Last edited: Jun 30, 2021
    • Agree Agree x 1
  5. This will result in the code working ONLY on Sittable animals.

    Improved code:
    Code (Java):
    if ((clickedEntity instanceof Tameable) && p.equals(((Tameable) clickedEntity).getOwner()) &&
        ((Tameable) clickedEntity).isTamed() && p.getInventory().getItemInMainHand().getType().equals(Material.STICK)) {
        if (clickedEntity instanceof Sittable) ((Sittable) clickedEntity).setSitting(false);
        ((Tameable) clickedEntity).setOwner(null);
        ((Tameable) clickedEntity).setTamed(false);
    }
     
  6. I edited to make it work on all tameables. Your code uses cast too much, cast is one of the most expensive things in java and should be avoided when possible, that's why I created a local variable so it casts only once.
     
  7. Thanks guys!