Please Critique My Script

[gothdad 1]

Here's my first script i've ever written. it's a basic script that buys then banks wizard mind bombs.  Please critique it.

 

import org.dreambot.api.methods.Calculations;
import org.dreambot.api.script.AbstractScript;
import org.dreambot.api.script.ScriptManifest;
import org.dreambot.api.script.Category;
import org.dreambot.api.wrappers.interactive.GameObject;
import org.dreambot.api.wrappers.interactive.NPC;
import org.dreambot.api.methods.map.Area;

@ScriptManifest(author = "gothdad", name = "WMBBuyer", version = 1.0, description = "Buys Wizard Mind Bombs in Falador", category = Category.MONEYMAKING)
public class main extends AbstractScript {

	private final Area BANK_AREA = new Area(2943, 3372, 2947, 3368);
	private final Area BAR_AREA = new Area(2955, 3375, 2960, 3366);

	public void onStart() {
		log("Script Started");
	}

	private enum State {
		BUY, BANK, WALKBANK, WALKBAR, WAIT,

	};

	private State getState() {
		if (getInventory().isFull() || getInventory().count("Coins") <= 3)
			if (BANK_AREA.contains(getLocalPlayer())) {
				return State.BANK;
			}

			else {
				return State.WALKBANK;
			}

		if (!getInventory().isFull()) // if player is in bar, buy wizard mind bombs
			if (BAR_AREA.contains(getLocalPlayer())) {
				return State.BUY;
			}

			else {
				return State.WALKBAR;

			}
		return State.WAIT;
	}

	public void onExit() {

	}

	@Override
	public int onLoop() {
		log(getState().toString());
		switch (getState()) {
		case BANK:
			GameObject bank = getGameObjects().closest("Bank booth");
			if (bank != null) {
				getMouse().click(bank, false);
			}
			if (getBank().isOpen()) {
				getBank().depositAll("Wizard's Mind Bomb");
			}
			if (getBank().isOpen() && getInventory().count("Coins") < 3 && getBank().count("Coins") > 81 ) {
				getBank().withdrawAll("Coins");
			}
			if (getBank().isOpen() && getInventory().count("Coins") < 3 && getBank().count("Coins") < 81 ) {
				log("you are too poor for this script");
			}

			break;

		case BUY:
			NPC kaylee = getNpcs().closest("Kaylee");
			if (!getDialogues().inDialogue() == true) {
				kaylee.interact("Talk-To");
				sleep(1000);
			}
			if (getDialogues().inDialogue() == true) {
				getDialogues().spaceToContinue();
				sleep(1000);
				getDialogues().spaceToContinue();
				sleep(1000);
				getDialogues().spaceToContinue();
				sleep(1000);
				getKeyboard().type("2");
				getDialogues().spaceToContinue();
				sleep(1000);
				getDialogues().spaceToContinue();
				sleep(1000);
				getDialogues().spaceToContinue();

			}

			else {
				log("need more coins");

			}

			break;

		case WALKBANK:
			getWalking().walk(BANK_AREA.getRandomTile());
			sleepUntil(() -> BANK_AREA.contains(getLocalPlayer()), 2000);
			break;

		case WALKBAR:
			getWalking().walk(BAR_AREA.getRandomTile());
			sleepUntil(() -> BAR_AREA.contains(getLocalPlayer()), 2000);
			break;

		case WAIT:
// WAIT
			break;

		}

		return Calculations.random(500, 600);
	}

}

 

[SubCZ 282]

Welcome  here's a list of improvements based on first glance:

• Use sleepUntil with an appropriate condition after performing an action whenever possible to avoid repeatedly interrupting and re-executing the same action.
• Use 'else if' to only perform one action per loop (saves performance from unnecessarily checking conditions).
• Randomize all your sleeps (a hard coded sleep(1000) makes the bot very detectable)
• Use bank.interact instead of getMouse.click (your bot will automatically use the right-click menu if there is another object in front of the bank this way)
• Use a non-uniform distribution for sleep times, preferably a skewed one (helps your bot stay undetected)
• (Optional, but recommended) Instead of relying on the return value of the loop for delays between actions, return a low value in the loop and place sleeps right before executing each action (make sure to recheck the action condition after the sleep); using the return value of the loop for delays makes it so that occasionally the loop cycle lines up perfectly with a condition becoming true which gives your bot superhuman reaction times.

You might want to separate your logic in different files as well when implementing this. Good luck!

 

[Xtra 31]

Hey, try not to use static sleeps, and basically what Sub said. I have a snippet for you, try to script in a way that you are not doing one action per loop, like so:
 

// Null check bank
if (bank != null) {
            
            // Interact with bank (instead of using Mouse)
            if (bank.interact()) {
                
                // If interact was successful and bank is open
                if (getBank().isOpen()) {
                    
                    // We only want coins in the inventory so we can buy mind bombs
                    if (getInventory().containsOnly("Coins")) {
                        
                        // Assuming we have coins, check if have enough for 1 inventory of mind bombs
                        if (getInventory().count("Coins") >= 81) {
                            getBank().close();
                            break;
                            
                        // If we don't have enough coins in our inventory, check if bank does
                        } else if (getBank().count("Coins") >= 81) {
                            getBank().withdrawAll("Coins");

                        } else {
                            log("you are too poor for this script");
                        }
                        
                        // Finally, if inventory doesn't only contain coins, bank all except coins
                        // It will loop again doing all checks and close the bank and break from bank case
                    } else {
                        getBank().depositAllExcept("Coins");
                    }
                }
            }
        }

Other than what was mentioned nice job