My first script(Ardy Knight pickpocket)

[Zendor 2]

Hello, i'm learning to script bots, and i made this one bc i hate pickpocketing ardy knights...

Features

• Pickpocket ardy knights at the bank
• Eat trout when health is below 35%
• Bank for more trout

Future features

• Eat whatever the person want

This is my very first bot, so i would like, if possible, some tips of what i can improve in my code. Thanks!!

Code: 

package ArdyPack;

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.NPC;


@ScriptManifest(name = "ArdyKngiht",version = 1.0,category = Category.THIEVING,author = "Zendor", description = "Pickpocket ardy knghts at the ardy bank(ONLY WORKS WITH TROUT ATM)")

public class ArdyKnight extends AbstractScript {
    @Override
    public int onLoop() {
        NPC knight = getNpcs().closest("Knight of Ardougne");
        Area bank = new Area(2655,3287, 2649, 3280, 0);
        if(bank.contains(getLocalPlayer())){
            if(getInventory().contains("Trout")){
                if(getCombat().getHealthPercent() < 35){
                    getInventory().interact("Trout", "Eat");
                    sleep(Calculations.random(1221, 1323));
                    getInventory().interact("Trout", "Eat");
                    sleep(Calculations.random(1221, 1323));
                    getInventory().interact("Trout", "Eat");
                }
                if(knight != null){

                    knight.interact("Pickpocket");
                    sleep(Calculations.random(524, 741));
                    knight.interact("Pickpocket");
                }
            }else{
                NPC banker = getNpcs().closest(npc -> npc != null && npc.hasAction("Bank"));
                if(banker.interact("Bank")){
                    if(sleepUntil(() -> getBank().isOpen(), 9000)){
                        getBank().depositAllExcept(item -> item != null && item.getName().contains("Coins"));
                        int countTrout = getInventory().count("Trout");
                        if(getBank().withdraw("Trout", 27)){
                            sleep(1200);
                            int countTrout2 = getInventory().count("Trout");
                            if(countTrout < countTrout2){
                                getBank().close();
                                if(sleepUntil(() -> !getBank().isOpen(), 5000)){
                                    knight.interact("Pickpocket");
                                    sleep(Calculations.random(510, 754));
                                    knight.interact("Pickpocket");
                                }
                            }
                        }
                    }
                }
            }
        }else{

            getWalking().walk(bank.getRandomTile());
        }
        return 510;
    }




}

[Nex 2542]

Not bad for a first script!

some points of advice:
would try to avoid repeating same stuff (eating)
add a few null checks what u did with banker u can apply to knight ( -> ..)
if(sleepUntil(() -> !getBank().isOpen(), 5000)){ (makes no sense)

[Zendor 2]

7 minutes ago, Nex said:

Not bad for a first script!

some points of advice:
would try to avoid repeating same stuff (eating)
add a few null checks what u did with banker u can apply to knight ( -> ..)
if(sleepUntil(() -> !getBank().isOpen(), 5000)){ (makes no sense)

When i learn how to have a graphic interface in my IDE i will make everything in classes to avoid repeating code. And the if(sleepUntil(() -> !getBank().isOpen(), 5000)) i did that to know if the bank is close, bc sometimes while i was testing it didnt close and the bot tries to pickpocket with the bank open, that was the only way i thought to solve it. Thanks for all the tips !!  

 

[Nex 2542]

52 minutes ago, Zendor said:

When i learn how to have a graphic interface in my IDE i will make everything in classes to avoid repeating code. And the if(sleepUntil(() -> !getBank().isOpen(), 5000)) i did that to know if the bank is close, bc sometimes while i was testing it didnt close and the bot tries to pickpocket with the bank open, that was the only way i thought to solve it. Thanks for all the tips !!  

 

Solution to this would be:
 

if (getBank().isOpen) {
    getBank().close();
    sleepUntil(() -> !getBank().isOpen(), 5000);
}

[ArmyofDragons 28]

5 hours ago, Nex said:

Solution to this would be:
 

if (getBank().isOpen) {
    getBank().close();
    sleepUntil(() -> !getBank().isOpen(), 5000);
}

Going off of what he is saying, I would also consider a sleepWhile instead of sleepUntil, because let's say there is an instance where the knight isn't *so close* to the bank, then we'd give it more time to see if he has run to the bank. Because if you do a sleepUntil, and 5 seconds pass, it won't check if the bank is not open, it will stop the conditional when the bank is open OR 5000 seconds have passed. So if you click on the bank and let's say the player is hindered by multiple obstacles and takes longer than expected, and 5 seconds pass, it will be as if you "assume" that the bank has been opened by that time. So you might want to make it sleepWhile so it actually does it when both 5000 seconds have passed and also the bank IS open. On top of that, I'd add some randomness to it, i.e:

 

if (getBank().isOpen) {
    getBank().close();
    sleepWhile(() -> !getBank().isOpen(), Calculations.random(4700, 6000);
}

 

If you want an easy explanation of what I mean (and the source of 'where' I got this from):

https://gyazo.com/f6b95b6ae4ad2b194da4c5c07f71f9a5

[Articron 738]

Thanks for your contribution to the open source community

Good job on making the effort to start out, keep at it my dude.

 

A comment  on the posts above: They forgot to mention that the Bank#close call also returns a boolean type that returns whether or not the action was successfully attempted.

With that in mind, you could make this even cleaner:

if (getBank().isOpen()) {
  if (getBank().close()) {
   sleepUntil(() -> !getBank().isOpen(), 600); //if the attempt was successful, the bank should close within one tick
  }
}

Hell, you could even inline this because of the boolean nature:

if (getBank().isOpen() && getBank().close()) {
    sleepUntil(() -> !getBank.isOpen(), 600);
}

The way control flow works is that java will perform a left-to-right check on your condition, meaning that if the first condition (isOpen) is false, it won't even bother to look at the second one (close).

Written on phone, excuse the bad identation

[Kocke 1]

Sweet release!

I have a pointer.

 public int onLoop() {
        NPC knight = getNpcs().closest("Knight of Ardougne");
        Area bank = new Area(2655,3287, 2649, 3280, 0);

Move Area bank(...) out of the onLoop, since you don't need to declare the area every loop because the area is the same.