Constructive Criticism on my Woodcutting Bot

[mshackertr2 0]

package main;

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

/**
 * Created by Jarod on 3/6/2020.
 */
@ScriptManifest(category = Category.WOODCUTTING, name = "Basic", author = "Jarod", version = 1.0)
public class Main extends AbstractScript {

    Area bankArea = new Area(1589, 3475, 1593, 3476);
    Area treeArea = new Area(1577, 3480, 1581, 3493);

    @Override
    public void onStart() {
        log("Hi");

    }

    @Override
    public int onLoop() {

        if (!getInventory().isFull()) {
            if (treeArea.contains(getLocalPlayer())) {
                chopTree("Magic tree");
            } else {
                if (getWalking().walk(treeArea.getRandomTile())) {
                    sleep(Calculations.random(3000, 5500));
                    if (treeArea != null) {
                        log("treeArea Chest is not null.");
                    } else {
                        log("treeArea Chest is null.");
                    }
                }
            }
        }

        if (getInventory().isFull()) {
            if (bankArea.contains(getLocalPlayer())) {
                bank();
            } else {
                if (getWalking().walk(bankArea.getRandomTile())) {
                    sleep(Calculations.random(3000, 6000));
                    if (bankArea != null) {
                        log("BankArea Chest is not null.");
                    } else {
                        log("BankArea Chest is null.");
                    }
                }
            }
        }

        return 600;
    }

    @Override
    public void onExit() {
        log("Bye");
    }

    @Override
    public void onPaint(Graphics graphics) {

    }

    private void chopTree(String treeName) {
        GameObject tree = getGameObjects()
                .closest(gameObject -> gameObject != null && gameObject.getName().equals(treeName));
        if (tree != null && tree.interact("Chop down")) {
            int countLog = getInventory().count("Logs");
            sleepUntil(() -> getInventory().count("Logs") > countLog, 12000);
        }
    }

    private void bank() {
        GameObject chest = getGameObjects().closest(c -> c != null && c.getName().contains("Bank chest") && c.hasAction("Use"));
        if (!getBank().isOpen()) {
            if (chest != null) {
                if (chest.interact("Use")) {
                    sleepUntil(() -> getBank().isOpen(), 2000);
                }
            }
        } else {
           if (getBank().depositAll(item -> item != null && item.getName().contains("Magic logs"))) {
                sleepUntil(() -> !getInventory().contains(item -> item != null && item.getName().contains("Magic logs")), 2000);
            }
        }

    }
}

[Gorn 4]

ok I will take a shot at it

if (!getInventory().isFull()) {
	//code
}

if(getInventory().isFull()){
	//code
}

You could just have else there

if (treeArea != null) {
	log("treeArea Chest is not null.");
} else {
	log("treeArea Chest is null.");
}

treeArea is your own variable that you define at the top, how could it be null?

if (getWalking().walk(treeArea.getRandomTile()))

you don't need to get a new random tile every time you call the walk function

@Override
public void onPaint(Graphics graphics) {

}

Why are you posting an empty function?

sleepUntil(() -> getInventory().count("Logs") > countLog, 12000);

if somebody else gets the tree before you the bot will just stand there until the sleepUntil times out

also sleepUntil is a performance hog

GameObject chest = getGameObjects().closest(c -> c != null && c.getName().contains("Bank chest") && c.hasAction("Use"));

Your code is written to only handle chests, why? There are 4 types of banks as you can see here

https://dreambot.org/javadocs/org/dreambot/api/methods/container/impl/bank/BankType.html

so let DreamBot handle this for you and write something like

BankLocation anyTypeOfBank = getBank().getClosestBankLocation();
getBank().open(anyTypeOfBank);

that way it will open the nearest bank regardless of the type

if (chest.interact("Use")) {
	sleepUntil(() -> getBank().isOpen(), 2000);
}

sleepUntil(() -> !getInventory().contains(item -> item != null && item.getName().contains("Magic logs")), 2000);

You use sleepUntil to check if the bank is open and also to check if the logs were already deposited. That's pretty expensive way of doing it.

while(!getBank().isOpen()){
	sleep(500);
}

This is essentially the same thing but without the performance hit, although it really only matters if you intend to run many bots.

[DefCon 121]

first remove that bankarea( we can reach it by applying the web walker )

i'd focus on seperating task1- chopTree & task2 -  walk from node1 - by state1 - haveLogs()

Switch(!haveLogs())  //State1 condition

then it's Task validation logic: 

Case true:
	if(treeArea.contains(getLocalPlayer()))  //TaskNode1 validation ; doubles as TaskNode 2's validation
		chopTree("Magic tree")   //TaskNode1 execution
    else getWalking().walk(treeArea);  //TaskNode2 execution

Case false:           

	BankLocation anyTypeOfBank = getBank().getClosestBankLocation(); getBank().open(anyTypeOfBank); //Node1 Execution

 

 

[TheCloakdOne 389]

BankLocation anyTypeOfBank = getBank().getClosestBankLocation(); getBank().open(anyTypeOfBank); //Node1 Execution

 

Careful with this line if you are botting around Varrock, under certain cases it will try and bank at the cooks guild even if you dont have the cooking level. Ideally whitelist a set of approved banks and only let it check against them:

 

    List<BankLocation> approvedBanks = Arrays.asList(BankLocation.VARROCK_EAST, BankLocation.VARROCK_WEST, BankLocation.GRAND_EXCHANGE);

    //Walk to bank
    BankLocation closestBank = this.getClosestBank();
    getBank().open(closestBank)
      
    //Get closest bank
    public static BankLocation getClosestBank() {
        double shortestDistance = 90000;
        BankLocation closestBank = null;

        for (BankLocation bank : approvedBanks) {
            if (bank.getCenter().distance(getLocalPlayer().getTile()) < shortestDistance) {
                shortestDistance = bank.getCenter().distance(getLocalPlayer().getTile());
                closestBank = bank;
            }
        }

        //If nothing is closest, go to first bank in list
        if (closestBank == null) {
            closestBank = approvedBanks.get(0);
        }

        log("Closest bank is: " + closestBank + " - shortestDistance away");
        return closestBank;
    }