Looking for feedback on my first script

[epicbotter69]

Hey guys I just wrote my first script, it's a very simple script for anglerfish on my main. I'd appreciate any feedback!

 

public final class Angler extends Script  {
    private final MouseTrail trail = new MouseTrail(0, 255, 255, 2000, this);
    private final MouseCursor cursor = new MouseCursor(52, 4, Color.white, this);
    private final Area fishingArea = new Area(1819,3765, 1840, 3780);

    @Override
    public final int onLoop() throws InterruptedException {
        if(!doingSomething()) {
            if(inventoryFull()) {
                if(Banks.PISCARILIUS_HOUSE.contains(myPosition())) {
                    bank();
                } else {
                    getWalking().webWalk(Banks.PISCARILIUS_HOUSE);
                }
            } else {
                if(hasCorrectEquipment()) {
                    if(fishingArea.contains(myPosition())) {
                        fish();
                    } else {
                        getWalking().webWalk(fishingArea);
                    }
                } else {
                    throw new InterruptedException("Please retry with bait and rod in inventory."); //Better exception?
                }
            }
        }

        return random(250,600);
    }

    @Override
    public void onPaint(Graphics2D g) {
        trail.paint(g);
        cursor.paint(g);
    }

    private void bank() throws InterruptedException {
        Bank bank = getBank();
        if (bank != null && bank.open()) {
            bank.depositAll("Raw anglerfish");
            if(getInventory().contains("Open fish barrel"))
            {
                getInventory().getItem("Open fish barrel").interact("Empty");
            }
            bank.close();
        }
    }

    private void fish() {
        Entity fishingSpot = getNpcs().singleFilter(getNpcs().getAll(), e -> e.getName().startsWith("Rod"));
        if (fishingSpot != null)
        {
            fishingSpot.interact("Bait");
        }
    }

    private boolean doingSomething() {
        return myPlayer().isAnimating() || myPlayer().isMoving();
    }

    private boolean inventoryFull() {
        return getInventory().getEmptySlots() == 0;
    }

    private boolean hasCorrectEquipment() {
        return getInventory().contains("Sandworms") && getInventory().contains("Fishing rod");
    }
}

I did have a couple of basic questions as well if you still have time after reviewing the code.

1. Why is anglerfish spot an NPC?
2. What is more 'human-like', always interacting with the closest fishing spot? Or doing what I did in fish() and just getting one that matches?
3. What exception to throw when user starts without correct inventory?
4. Cleaner onLoop() implementation ideas? This bot is very simple but I already hate the amount of if..else... nested everywhere, Maybe a list of objects with "shouldRun()" and "run()"?

[Gunman]

21 minutes ago, epicbotter69 said:

 

1. Why is anglerfish spot an NPC?
2. What is more 'human-like', always interacting with the closest fishing spot? Or doing what I did in fish() and just getting one that matches?
3. What exception to throw when user starts without correct inventory?
4. Cleaner onLoop() implementation ideas? This bot is very simple but I already hate the amount of if..else... nested everywhere, Maybe a list of objects with "shouldRun()" and "run()"?

1. Jagex made it an NPC in the game code, ask them why.
2. Mixture of the one that's easiest to see with the eyes at a glance, and the one closest to the mouse.
3. Either have it spam the issue in the logger, or stop the script and give the reason the script stopped. Most normal users won't know what exceptions are, and not really a reason for it here.
4. Probably mean this I guess 

 

[Spork]

Hey dude, nice script! But you know, you can also add another bank function to acquire the stuff that's needed, right? Like something like this:

	private void equipBank() throws InterruptedException {
		Bank bank = getBank();
		if (bank != null && bank.open()) {
			bank.depositAll();
			if (!getInventory().contains("Sandworms") && getInventory().contains("Fishing rod")) {
				if (bank.contains("Sandworms") && bank.contains("Fishing rod")) {
					if (!inventory.contains("Fishing rod")) {
						bank.withdraw("Fishing rod", 1);
					}
					if (!inventory.contains("Sandworms")) {
						bank.withdrawAll("Sandworms");
					}
				}
			} else {
				//EXIT SCRIPT OR ADD GE FUNCTIONALITY
			}
		}
		bank.close();
	}

That would give you a better function to resolve in your else, but I'd recommend throwing it into its own else if at the second from the top after your location resolve, and then adding another else to log("Weird event?"} or something and stop() if there's some sort of out of scope event

 

Also, 

    private boolean inventoryFull() {
        return getInventory().getEmptySlots() == 0;
    }

is redundant as you can always use

getInventory().isFull()

as part of the Inventory class.

 

And as for #4, I agree with @Gunmanand also use that task framework in all of my scripts

 

Keep it up though, this is a pretty robust first script, and I can't wait to see what you write when you're more comfortable with the platform

Edited April 2, 2022 by Spork