Jump to content
Frequently Asked Questions
  • Are you not able to open the client? Try following our getting started guide
  • Still not working? Try downloading and running JarFix
  • Help! My bot doesn't do anything! Enable fresh start in client settings and restart the client
  • How to purchase with PayPal/OSRS/Crypto gold? You can purchase vouchers from other users
  • Roast my code


    fiesch

    Recommended Posts

    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

     

    Link to comment
    Share on other sites

    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

    Link to comment
    Share on other sites

    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){
      ...

     

    Link to comment
    Share on other sites

    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()?

     

     

    Link to comment
    Share on other sites

    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?

    Link to comment
    Share on other sites

    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

    Link to comment
    Share on other sites

    Archived

    This topic is now archived and is closed to further replies.

    ×
    ×
    • Create New...

    Important Information

    We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.