Roast my code

[fiesch 4]

Hi All

As some of you might know I just joined the community and just started today with creating bots for runescape. 

So please roast my code so i can improve. I was unsure of a lot and for sure did a lot of stuff in the wrong way.

Edit: I developed a simple woodcutter as a hello world project that starts of in the starting location cops down trees till he is level 15 and then he starts chopping down oaks.

 https://github.com/sadfiesch/FieschWoodcutter

 

[brave 29]

[fiesch 4]

@braveheartFixed was set to private. Thanks for the response

[osrssom 25]

In banking, instead of

if (bankBooth.interact("Bank")) {
                sleep(Calculations.random(350, 700)); //todo make better
            }

use

if (bankBooth.interact("Bank")) {
                sleepUntil(() -> Bank.isOpen(), 4000);
            }

 

When dropping all inventory items you can create drop patterns to make the bot more human-like.

 

Instead of

Walking.walk(WoodCutter.stairsArea.getRandomTile());
            sleep(Calculations.random(350, 700)); //todo make better

try

do {
	Walking.walk(WoodCutter.stairsArea.getRandomTile());
    sleepUntil(WoodCutter.stairsArea.contains(getLocalPlayer()));
} while (!WoodCutter.stairsArea.contains(getLocalPlayer()));

^You should always use a do-while loop for walking, it would be good to implement this throughout the rest of your script.

 

Overall, good start and good structure

[fiesch 4]

Just did a major update https://github.com/sadfiesch/FieschWoodcutter hate on it if you can

[trtl 2]

I don't consider myself a scripter really yet, so just a very minor thing, but you could move this (in your chop trees task) into your accept conditions:

...
  if (tree == null){
  ...

 

[camelCase 301]

On 10/19/2021 at 5:35 AM, osrssom said:

try

do {
	Walking.walk(WoodCutter.stairsArea.getRandomTile());
    sleepUntil(WoodCutter.stairsArea.contains(getLocalPlayer()));
} while (!WoodCutter.stairsArea.contains(getLocalPlayer()));

^You should always use a do-while loop for walking, it would be good to implement this throughout the rest of your script.

 

while loops are advised against because they easily make your script hang & theres no reason to use that how he has his walking nodes set up, the way hes doing it is fine & imo better than using a while loop, the script itself is already a loop you dont need a new one.

 

right now you have it sleepUntil(() -> Util.currentZone.getBankArea().contains(getLocalPlayer()), 2000); i think if you had

public int execute() {
        log("Moving to bank area");
        if(!Util.currentZone.getBankArea().contains(getLocalPlayer())) {
			if(Walking.shouldWalk()) {
            	Walking.walk(Util.currentZone.getBankArea().getRandomTile());
        	}
		}
        return Calculations.random(75, 450);
    }

forgive the fucked up indentation but that would probably produce cleaner walking i havent seen how ur code walks so idk

personally i wouldnt have tasknodes for walking id just wrap other tasks eg the chopping task in, i dont think it makes a huge difference just nicer codebase imo

if (*at the chopping area*) {

// chop shit

} else {

// walking code

}

 

 

method isChopping(), isCombat() & isMoving() are redundant idk why you made those

 

in your banking node you have an if (Bank.isOpen()) & if(!Bank.isOpen) instead of an if else, i just think you're a sociopath for doing that

 

theres no need to get the gameobject of the bankbooth just use Bank.openClosest()

 

in GrandExchangeTask you have 

                if(Bank.getClosestBank(BankType.EXCHANGE).interact("Bank")){
                    sleepUntil(() -> Bank.isOpen(), 2000);
                }

just Bank.openClosest()?

 

 

[wr80 15]

So lemme ask this cause I suck at java, is the TaskNode a better structure to coding than tossing most the code in one giant class?

[camelCase 301]

12 hours ago, wr80 said:

So lemme ask this cause I suck at java, is the TaskNode a better structure to coding than tossing most the code in one giant class?

its just easier to read and maintain

[wr80 15]

11 hours ago, camalCase said:

its just easier to read and maintain

Cool! I guess that makes sense.