First Script Feedback

[bubbajim]

Hi guys. I just got into script making yesterday. I finished my first script today at the request of someone. Its a cadava berry picker that banks. I would appreciate any feedback and constructive criticism. Thank you!

 

import org.osbot.rs07.api.Settings;
import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.Position;
import org.osbot.rs07.api.model.Entity;
import org.osbot.rs07.api.ui.Tab;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;
import java.awt.*;
import java.util.concurrent.TimeUnit;

@ScriptManifest(name = "Bubba's Berries", author = "bubbajim", version = 1.0, info = "", logo = "")

public class BubbasBerries extends Script {
    private long startTime;
    private int berriesCounter;
    private int startWorld = 460;
    private int endWorld = 451;
    private int currentWorld = startWorld;
    private long currentInventoryCapacity = -5;

    Position pathInBetween = new Position(3290, 3397, 0);
    Position pathFromBank = new Position(3273, 3370, 0);
    Position pathToBank = new Position(3252, 3420, 0);

    Area bankArea = new Area(new Position(3257, 3423, 0),
            new Position(3250, 3420, 0));

    @Override
    public void onStart() {
        startTime = System.currentTimeMillis();
    }

    @Override
    public void onExit() {
    }

    @Override
    public int onLoop() throws InterruptedException {
        if (getInventory().getAmount(753) == currentInventoryCapacity + 1) {
            berriesCounter += 1;
        }
        currentInventoryCapacity = getInventory().getAmount(753);

        Settings settings = getSettings();
        int runEnergy = settings.getRunEnergy();
        if (runEnergy > 50 && !settings.isRunning()) {
            settings.setRunning(true);
        }

        if (getInventory().isFull()) {
            getWalking().webWalk(pathInBetween);
            getWalking().webWalk(pathToBank);
            getBank().open();
            sleep(random(1500,2500));
            getBank().depositAll();
            new ConditionalSleep(10000) {
                @Override
                public boolean condition() {
                    return getInventory().isEmpty();
                }
            }.sleep();
        }

        Entity cadavaBush = getObjects().closest(23625, 23626);

        if (bankArea.contains(myPosition())) {
            getWalking().webWalk(pathInBetween);
            getWalking().webWalk(pathFromBank);
        }

        else if (cadavaBush != null && cadavaBush.interact("Pick-from")) {
            int cadavaBushX = cadavaBush.getX();
            int cadavaBushY = cadavaBush.getY();
            Position cadavaBushPosition = new Position(cadavaBushX, cadavaBushY, 0);

            if (cadavaBush.getId() == 23625) {
                new ConditionalSleep(10000) {
                    @Override
                    public boolean condition() {
                        return getObjects().closest(o -> o.getPosition().equals(cadavaBushPosition)).getId() == 23626;
                    }
                }.sleep();
            } else if (cadavaBush.getId() == 23626) {
                new ConditionalSleep(10000) {
                    @Override
                    public boolean condition() {
                        return getObjects().closest(o -> o.getPosition().equals(cadavaBushPosition)).getId() == 23627;
                    }
                }.sleep();
            }
        } else {
            if (endWorld == currentWorld) {
                currentWorld = startWorld;
            }
            currentWorld -= 1;
            getTabs().open(Tab.LOGOUT);
            getWorlds().hop(currentWorld);
            sleep(random(2000,3000));
            getTabs().open(Tab.INVENTORY);
            new ConditionalSleep(10000) {
                @Override
                public boolean condition() {
                    return getWorlds().getCurrentWorld() == currentWorld;
                }
            }.sleep();
        }
        return random(300, 600);
    }

    @Override
    public void onPaint(Graphics2D g) {
        final long runTime = (System.currentTimeMillis() - startTime);
        String hms = String.format("%02d:%02d:%02d", TimeUnit.MILLISECONDS.toHours(runTime),
                TimeUnit.MILLISECONDS.toMinutes(runTime) % TimeUnit.HOURS.toMinutes(1),
                TimeUnit.MILLISECONDS.toSeconds(runTime) % TimeUnit.MINUTES.toSeconds(1));
        Font font = new Font("Open Sans", Font.PLAIN, 12);
        g.setFont(font);
        g.setColor(Color.green);
        g.drawString("Time elapsed: " + hms, 15, 300);
        g.drawString("Berries picked: " + berriesCounter, 15, 315);
    }
}

 

[Hybris]

13 hours ago, bubbajim said:

 

public class BubbasBerries extends Script {
    @Override
    public int onLoop() throws InterruptedException {
        if (getInventory().getAmount(753) == currentInventoryCapacity + 1) {
            berriesCounter += 1; //Not really important but you can change this to berriesCounter++;
        }
	//Using the name instead of the ID makes it way more readable & gives the same results. Also names will (or at least should) never change.
        currentInventoryCapacity = getInventory().getAmount(753); 
      

      //You're initializing a bunch of things although you're only using them once.
        Settings settings = getSettings(); //You can just use "settings.whateverFunction() or getSettings.whateverFunction(). No need to initliaze it
        int runEnergy = settings.getRunEnergy();
        if (runEnergy > 50 && !settings.isRunning()) {
            settings.setRunning(true);
        }
    //You can rewrite the ENTIRE block above to this line:
	//if(settings.getRunEnergy() > 50 && !settings.isRunning()) settings.setRunning(true)

        if (getInventory().isFull()) {
			//double webWalking to a position? You could replace this to just webwalking to the bank area. 
            //Also if there are no obstaclesdon't use webwalking, use the normal walk instead as it has better performance
            getWalking().webWalk(pathInBetween); 
            getWalking().webWalk(pathToBank); 
            getBank().open();
            sleep(random(1500,2500));
            getBank().depositAll();
			//Use Lambda Expressions for conditional sleeps instead, it looks way cleaner. will link below.
          	//Example for this one would be: Sleep.sleepUntil(() -> getInventory().isEmpty(), 10000);
            new ConditionalSleep(10000) { 
                @Override
                public boolean condition() {
                    return getInventory().isEmpty();
                }
            }.sleep();
         
        }

        Entity cadavaBush = getObjects().closest(23625, 23626);

        if (bankArea.contains(myPosition())) {
          //double webwalking again, any reason?
            getWalking().webWalk(pathInBetween);
            getWalking().webWalk(pathFromBank);
        }
		else if (cadavaBush != null && cadavaBush.interact("Pick-from")) {
          //You're only using these values once, no need to initialize them.
          //Use this instead: Position cadavaBushPosition = new Position(cadavaBush.getX(), cadavaBush.getY());
            int cadavaBushX = cadavaBush.getX();
            int cadavaBushY = cadavaBush.getY();
            Position cadavaBushPosition = new Position(cadavaBushX, cadavaBushY, 0);
			
            if (cadavaBush.getId() == 23625) {
              //Use Lambda Expression for CS
                new ConditionalSleep(10000) {
                    @Override
                    public boolean condition() {
                        return getObjects().closest(o -> o.getPosition().equals(cadavaBushPosition)).getId() == 23626;
                    }
                }.sleep();
            } else if (cadavaBush.getId() == 23626) {
				//Use Lambda Expression for CS
                new ConditionalSleep(10000) {
                    @Override
                    public boolean condition() {
                        return getObjects().closest(o -> o.getPosition().equals(cadavaBushPosition)).getId() == 23627;
                    }
                }.sleep();
            }
        } else {
            if (endWorld == currentWorld) {
                currentWorld = startWorld;
            }
            currentWorld -= 1;
            getTabs().open(Tab.LOGOUT);
            getWorlds().hop(currentWorld);
          //Use conditional sleep, you can use 'getClient().getLoginState().equals(Client.LoginState.LOGGED_IN))' to check if you are logged in
            sleep(random(2000,3000));
            getTabs().open(Tab.INVENTORY);
            new ConditionalSleep(10000) {
                @Override
                public boolean condition() {
                    return getWorlds().getCurrentWorld() == currentWorld;
                }
            }.sleep();
        }
        return random(300, 600);
    }

    @Override
    public void onPaint(Graphics2D g) {
        final long runTime = (System.currentTimeMillis() - startTime);
        String hms = String.format("%02d:%02d:%02d", TimeUnit.MILLISECONDS.toHours(runTime),
                TimeUnit.MILLISECONDS.toMinutes(runTime) % TimeUnit.HOURS.toMinutes(1),
                TimeUnit.MILLISECONDS.toSeconds(runTime) % TimeUnit.MINUTES.toSeconds(1));
        Font font = new Font("Open Sans", Font.PLAIN, 12);
        g.setFont(font);
        g.setColor(Color.green);
        g.drawString("Time elapsed: " + hms, 15, 300);
        g.drawString("Berries picked: " + berriesCounter, 15, 315);
    }
}

 

I added a bunch of comments that will hopefully help you on your way

- Hybris

Edited August 15, 2019 by Hybris

[bubbajim]

6 hours ago, Hybris said:

I added a bunch of comments that will hopefully help you on your way

- Hybris

Hey thanks a lot for the feedback. I really appreciate it. All the comments you made are super informative and i will refactor my script based on them.

Ill just go ahead and address this one comment you made:

//double webWalking to a position? You could replace this to just webwalking to the bank area. 
//Also if there are no obstaclesdon't use webwalking, use the normal walk instead as it has better performance

I used double webWalking because sometimes it walks by the dark wizards so i figured if i set an intermediate point it will always stick to the Varrock west path. Also i tried using normal walk but it was really buggy for me. It would walk to a tile and just spam click it and not go to the next tile in the path i defined. I'm not sure why this is happening? Maybe i'm using it wrong?

[Hybris]

4 hours ago, bubbajim said:

I used double webWalking because sometimes it walks by the dark wizards so i figured if i set an intermediate point it will always stick to the Varrock west path. Also i tried using normal walk but it was really buggy for me. It would walk to a tile and just spam click it and not go to the next tile in the path i defined. I'm not sure why this is happening? Maybe i'm using it wrong?

Did you use walking.walk() or walking.walkPath()? if using the walkPath it's probably because your positions are too far apart from eachother, try to place them closer to eachother
If you don't mind performance (like you don't want to run a botfarm of them) it doesn't really matter to use the (double) webwalk but if you do it's probably better to make a solid walkPath

Feel free to post a reworked version of the script below & I'll take another look at it, I'm not 100% confident in my Java skills & might not be using all the best practices so it's probably a good idea to have another pair of eyes on it.

- Hybris