Script help needed! [What not to do & how I can do it better]

[Chris]

  @Override
    public int onLoop() throws InterruptedException {
        if (!inventory.contains("Lobster")&&MONSTER_AREA.contains(myPlayer())){
            if (!BANK_AREA.contains(myPlayer())){
                getLocalWalker().walkPath(PATH_TO_BANK); //walk to bank
            }else{
                RS2Object bankBooth = (RS2Object)this.objects.closest(new String[] {"Bank booth"});
                if (bankBooth != null){
                    if (bankBooth.interact("Bank"))
                    { //checks if it will return true
                        new ConditionalSleep(10000) {
                            @Override
                            public  boolean condition() throws InterruptedException {
                                return getBank().isOpen();
                            }
                        }.sleep();
                        if (getBank().isOpen()){
                            getBank().withdrawAll("Lobster");
                            sleep(random(377, 544));
                        }
                    }
                    bank.close();
                    log("STATE:. BANKED");
                }
            }
        }
        if (!MONSTER_AREA.contains(myPlayer())){
            getLocalWalker().walkPath(PATH_TO_MONSTER);
            log("STATE: WALKING BACK");
        }

This is the code I am dealing with. I want it to go from Varrock Palace to Varrock west bank and use the booth. Then head back to the guards. What am I doing wrong here and how can I improve my code?

[Isolate]

1. The moment you leave monster area you'll stop walking.
2.  

if(bank.isOpen(){
  //do banking
}else{
  bank.open();
}

is prolly more secure

should end this 

if (!inventory.contains("Lobster")&&MONSTER_AREA.contains(myPlayer())){

with an else like 

if (!inventory.contains("Lobster")){
  //bank cheecks
  //walk to bank
  //bank ect
}else{
 //fighting checks
 //walk to fight
 //fight ect
}

[Chris]

 

1. The moment you leave monster area you'll stop walking.

2.  

if(bank.isOpen(){
  //do banking
}else{
  bank.open();
}

is prolly more secure

should end this 

if (!inventory.contains("Lobster")&&MONSTER_AREA.contains(myPlayer())){

with an else like 

if (!inventory.contains("Lobster")){
  //bank cheecks
  //walk to bank
  //bank ect
}else{
 //fighting checks
 //walk to fight
 //fight ect
}

Thanks for the response!  

[chad0ck]

Thanks for the response!

Actually, I'd say there is nothing wrong with the banking you have. 

if (inventory.isFull() && BANK_AREA.contains(myPlayer().getPosition())) {

if (inventory.isFull()) {
return State.GO_TO_BANK;
}

This is part of my fighting script. It switches to the correct states when needed, I don't see how it can be improved. 

Edited July 31, 2015 by chad0ck

[Bobrocket]

Actually, I'd say there is nothing wrong with the banking you have. 

if (inventory.isFull() && BANK_AREA.contains(myPlayer().getPosition())) {

if (inventory.isFull()) {
return State.GO_TO_BANK;
}

This is part of my fighting script. It switches to the correct states when needed, I don't see how it can be improved. 

 

I can: use nodes

public abstract class _Node
{
    protected Script script;
    public _Node(Script script)
    {
        this.script = script;
    }

    public abstract boolean canProcess();
    public abstract void process();

    public void run()
    {
        if (canProcess()) process();
    }
}

public class Bank extends _Node
{
    public Bank(Script script)
    {
        super(script);
    }

    @Override
    public boolean canProcess()
    {
        return script.getInventory().isFull();
    }

    @Override
    public void process()
    {
        //stuff
    }
}


//in onStart
List<_Node> nodes = new ArrayList<_Node>();
nodes.add(new Bank(this));

//in onLoop
nodes.forEach(node -> node.run());

[chad0ck]

 

I can: use nodes

public abstract class _Node
{
    protected Script script;
    public _Node(Script script)
    {
        this.script = script;
    }

    public abstract boolean canProcess();
    public abstract void process();

    public void run()
    {
        if (canProcess()) process();
    }
}

public class Bank extends _Node
{
    public Bank(Script script)
    {
        super(script);
    }

    @Override
    public boolean canProcess()
    {
        return script.getInventory().isFull();
    }

    @Override
    public void process()
    {
        //stuff
    }
}


//in onStart
List<_Node> nodes = new ArrayList<_Node>();
nodes.add(new Bank(this));

//in onLoop
nodes.forEach(node -> node.run());

 

I figured he was still trying to learn the process, if I were him i'd stick to states, they are easy to understand and after you have them down you can learn new processes

[Bobrocket]

I figured he was still trying to learn the process, if I were him i'd stick to states, they are easy to understand and after you have them down you can learn new processes

 

It's better to learn things like this early on, it's simple and it's gonna remove a lot of redundant code (1000+ lines in onLoop was mainstream anyway, right guys?)

[chad0ck]

It's better to learn things like this early on, it's simple and it's gonna remove a lot of redundant code (1000+ lines in onLoop was mainstream anyway, right guys?)

I mean you do got a point there.. lol

[Chris]

Meh I tried states but I gave up on them

[chad0ck]

Meh I tried states but I gave up on them

They are actually very easy to understand, giving up won't get you anywhere!