Help me improve my script, and use Lambdas?

slazter · July 11, 2017

Okay so i've been coding for not that long, mabye 2-3 weeks, but i see alot of really good coders around here, and what i can tell is that the better coder you are, the bigger the toolset of your coding.
So the last few days i've been trying to improve my coding with making a questing script, which both improved my understanding of whenever to make a method for repeating task, passing arguments, aswell as overloading methods. However when i look at other scripters i se that the really good ones use lambdas when it's suitable, and i want to be able to do that aswell, as it makes for really smooth code and seem easier to manage. I've understood some parts of it, as on how to make a Lambda, and by that i mean making a functional interface that the Lambda can implement use as a type, aswell as on how to write it's syntax. What i've not fully grasped is how to use it in the context of my own scripts..

So what i really would like input on is, do you se any feature that i could extract further or improve?
1)Is there any code i should try to make a method of instead?
2)is there some code i should put in a superclass or interface? (Any input on why, and when that is suitable would also be super helpful)
3)Is there some part i can turn into a Lamda statement? I'm personally thinking of the dialogueV2 parts, but not quite sure on how to.. this is would really really make me happy if someone could provide help on
4)My main questing script is atm 630 lines, and involves 8-9 quests, I intend to script pretty much every quest(which is going to make it really big if i keep it up this way) , so should i try to like make some kind of superclass, and make the subquests children? or mabye some other smart solution?
5) If yes on number 4, would it make sense to make an abstract class, and mabye have dialoguehandling, and dialoguehandlingV2, in there as concrete methods, so i could just make an object of that class to implement them without really cluttering up to much of my code?
6) So i have a class called "Platser" where i save many areas or positions around runescape, however i read that when u create an object, the constructor makes space for the object on the heap for that object based on the constructor, does that mean it's bad practice for me to make a new object to call my spots more easily (cause the object have to store all the diffrent spots?) rather then just making them method variables, to save memory?
7) Any other coder out there that want to talk on skype with me to exchange ideas, if so feel free to add me on skype: Slazter

My code to give input on:

import org.osbot.rs07.api.filter.Filter;
import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.Position;
import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.api.model.Item;
import org.osbot.rs07.api.model.NPC;
import org.osbot.rs07.api.model.RS2Object;
import org.osbot.rs07.api.ui.Tab;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

import static org.osbot.rs07.api.Quests.Quest.THE_KNIGHTS_SWORD;

@ScriptManifest(author = "slazter", name = "Knights Sword f2p q", info = "Does Knights sword if u have all items", version = 0.1, logo = "")

public final class KnightsSword extends Script  {

    @Override
    public final int onLoop() throws InterruptedException {
        if(canKnight()){
            questKnightsSword();
        }
        return (random(150,250));
    }

    //Method for dialoguehandling when not needing to give extra input
    public void dialogueHandling() throws InterruptedException {
        String[] options = {""};
        if (dialogues.isPendingContinuation()) {
            dialogues.clickContinue();
        } else if (dialogues.isPendingOption()) {
            dialogues.completeDialogue(options);
        }
    }
    //method for when i want to supply the talking options,
    public void dialogueHandling(String[] opt) throws InterruptedException {
        String[] options = opt;
        if (dialogues.isPendingContinuation()) {
            dialogues.clickContinue();
        } else if (dialogues.isPendingOption()) {
            dialogues.completeDialogue(options);
        }
    }

    //method for making it easier to select npc - default dialogue options
    public void dialogueV2(NPC name) throws InterruptedException {
        NPC qNpc;
        qNpc = name;
        if (Tab.INVENTORY.isDisabled(this.getBot())) {
            Sleep.sleepUntil(() -> !Tab.INVENTORY.isDisabled(this.getBot()), 15000);
        } else if (!dialogues.inDialogue()) {
            qNpc.interact("Talk-to");
            Sleep.sleepUntil(() -> dialogues.inDialogue(), 5000);
        } else if (dialogues.inDialogue()) {
            dialogueHandling();
        }
    }
    //method for making it easier to select npc - With supplied talk options
    public void dialogueV2(NPC name, String[] opts) throws InterruptedException {
        NPC qNpc;
        qNpc = name;
        String[] options = opts;
        if (Tab.INVENTORY.isDisabled(this.getBot())) {
            Sleep.sleepUntil(() -> !Tab.INVENTORY.isDisabled(this.getBot()), 15000);
        } else if (!dialogues.inDialogue()) {
            qNpc.interact("Talk-to");
            Sleep.sleepUntil(() -> dialogues.inDialogue(), 5000);
        } else if (dialogues.inDialogue()) {
            dialogueHandling(options);
        }
    }
    
    //bolean to see if quest isn't done
    public boolean canKnight() {
        if (!quests.isComplete(THE_KNIGHTS_SWORD)) {
            return true;
        } else return false;
    }

    //the main quest method
    public void questKnightsSword() throws InterruptedException {
        String[] val = {"Something else.","Would you like some redberry pie?"};
        RS2Object bluerite = getObjects().closest(7495);
        NPC squire = getNpcs().closest("Squire");
        NPC thurgo = getNpcs().closest("Thurgo");
        NPC reldo = getNpcs().closest("Reldo");
        String[] qItems = {"Iron bar", "Redberry pie"};
        Area squire_spot = new Area(2980, 3339, 2975, 3344);
        Area thurgo_spot = new Area(3001, 3143, 2995, 3146);
        Area reldo_spot = new Area(3213, 3490, 3207, 3497);
        Filter<Item> pickaxe = new Filter<Item>() {
            @Override
            public boolean match(Item item) {
                return item.getName().contains("pickaxe");
            }
        };
        
        //withdraw q items if not in inv
        if (!inventory.contains(qItems) && configs.isSet(122,0)) {
            if (!getBank().isOpen()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
                getBank().open();
            } else if (getBank().isOpen()) {
                getBank().withdraw("Iron bar", 2);
                getBank().withdraw("Redberry pie", 1);
                getBank().withdraw(pickaxe, 1);
            }
        } else if (configs.isSet(122,0)) {
            if (squire != null) {
                dialogueV2(squire);
            } else
                getWalking().webWalk(squire_spot);
        } else if (getConfigs().isSet(122,1)) {
            if(reldo!=null){
                if(!dialogues.inDialogue()){
                    reldo.interact("Talk-to");
                    sleep(random(1200,1800));
                } else if(dialogues.inDialogue()){
                    if(dialogues.isPendingContinuation()){
                        dialogues.clickContinue();
                    } else if(dialogues.isPendingOption()){
                        String[] options = {"What do you know about the Imcando dwarves?"};
                        dialogues.completeDialogue(options);
                    }
                }
            }
            else getWalking().webWalk(reldo_spot);
        }
        else if(configs.isSet(122,2)){
            String[] opts = {"Something else.","Would you like some redberry pie?"};
            if(thurgo!=null){
                dialogueV2(thurgo,opts);
            } else
                getWalking().webWalk(thurgo_spot);
        }
        else if(configs.isSet(122,3)){
            String[] opts = {"Something else."};
            if(thurgo!=null){
                dialogueV2(thurgo,opts);
            }
        }
        else if(configs.isSet(122,4)){
            if(squire!=null){
                dialogueV2(squire);
            } else getWalking().webWalk(squire_spot);
        }
        else if(configs.isSet(122,5)){
            String[] food = {"Trout"};
            RS2Object cup = getObjects().closest("Cupboard");

            if(inventory.contains("Portrait")) {
                //get food before getting near ice warriors
                if(!inventory.contains(food)){
                    if (!getBank().isOpen()){
                        getWalking().webWalk(Banks.FALADOR_EAST);
                        getBank().open();
                    }
                    else if(getBank().isOpen()) {
                        getBank().withdraw("Trout", 4);
                        getBank().close();
                    }
                }
                else if(thurgo!=null){
                    dialogueV2(thurgo,val);
                }
                else getWalking().webWalk(thurgo_spot);
            }
            else if(cup!=null && getMap().canReach(cup)){
                if(getDialogues().isPendingContinuation()){
                    dialogues.clickContinue();
                }
                cup.interact("Open");
                sleep(random(1500,2100));
                cup.interact("Search");
            } else
                getWalking().webWalk(new Position(2984,3335,2));
        } else if(configs.isSet(122,6)){
            if(inventory.contains("Blurite sword")){
                if(squire!=null){
                    dialogueV2(squire);
                } else
                    getWalking().webWalk(squire_spot);
            }
            else if(!inventory.contains("Blurite ore")){
                if(bluerite!=null && !myPlayer().isAnimating()){
                    bluerite.interact("Mine");
                    Sleep.sleepUntil(()-> myPlayer().isAnimating(),5000);
                } else
                    getWalking().webWalk(new Position(3049,9567,0));
            } else if(thurgo!=null) {
                dialogueV2(thurgo,val);
            } else getWalking().webWalk(thurgo_spot);
        }
    }
}

**Tom** · July 11, 2017

My first impression is that this is pretty messy

I'm about to head off to bed, but one of the first things I see is related to efficiency / poor design

        Area squire_spot = new Area(2980, 3339, 2975, 3344);
        Area thurgo_spot = new Area(3001, 3143, 2995, 3146);
        Area reldo_spot = new Area(3213, 3490, 3207, 3497);

You are creating a new Area every time the method is called, you should initialise them in the onStart or in a constructor, and then just call them by their variable name.
This increases the variables scope, and is better design for what you are doing in my opinion.

http://www.javawithus.com/tutorial/scope-and-lifetime-of-variables

Area squires_spot;

 

@Override

public void onStart(){

    squire_spot = new Area(2980, 3339, 2975, 3344);

}

Thats just a quick example, I'll drop back by tomorrow but I'm sure @Explv will have sorted you out by then.

As for any new scripter, feel free to add me on skype 'tommyhoogstra'
I'll offer you my free advice and opinions, but you need to be able to take constructive criticism well.

Edited July 11, 2017 by Tom

Explv · July 11, 2017

A few things I notice that are bad:

You don't need to create your own methods for handling dialogue. There already exists a method getDialoges().completeDialogue(options) that will click continue and select options etc.
Instead of having if statements for checking configs, you could just use a switch, it would be much cleaner:

switch(getConfigs().get(123)) {
  case 0:
    doSomething();
    break;
  case 1:
    doSomethingElse();
    break;
  // etc.
}

In your questKnightsSword method you have a bunch of Areas. Those should be global final variables, as you are never changing them, and you don't want to keep creating new instances every time that method is called.
The above also applies to the pickaxe filter, val and qItems variables in your questKnightsSword method
In the questKnightsSword method, at the start, you always search for three NPCs. You should only search for an NPC if you are going to use it, e.g. only get the squire npc when you are going to talk to the squire.
Some of your variables are poorly named, such as "val". Use proper naming conventions
You should check the return status of an API call. For example you interact with the blurite rock and then sleep, but you don't actually check if the interaction was successful. If the interaction failed for some weird reason, the script would just sleep for 5 seconds:

bluerite.interact("Mine");
Sleep.sleepUntil(()-> myPlayer().isAnimating(),5000);

Should be:

if (bluerite.interact("Mine")) {
    Sleep.sleepUntil(()-> myPlayer().isAnimating(), 5000);
}

Similar to the point above, you have a bunch of sequential API calls in your code, assuming that the previous call was successful. API calls CAN fail, and you should account for it. For example when you withdraw "q items" you just withdraw one after the other, not accounting for if the previous one failed and you needing to withdraw again.
dialogueV2 is a terrible name for a method
You have some elses without curly braces which could easily result in you messing something up
You also have a few sleeps that are just sleep(random(int, int)). These should all really be replaced by a ConditionalSleep

Regarding your first question, I would clean up your questKnightsSword method, there's quite a few areas in there that could benefit from being moved out into their own methods.

As for you wanting to learn more about lambdas, you are already making use of them in the Sleep.sleepUntil method. You could also use a lambda expression for the pickaxe filter:

Filter<Item> pickaxe = item -> item.getName().contains("pickaxe");

Because you are making a quest script with a bunch of quests, you should focus on studying OOP concepts. There are a lot of common things you have to do in quests, so you should be making proper use of inheritance, utility classes / methods etc. to reduce the amount of code you have to write.

Regarding your 6th point "So i have a class called "Platser" where i save many areas or positions around runescape", I think you should store areas and positions in the places where you are going to use them. The only reason to create a class with variables like that is if you are using them in multiple places, for example the Banks class in the OSBot API.

14 minutes ago, Tom said:

I'll drop back by tomorrow but I'm sure @Explv will have sorted you out by then.

Damn, I have become too predictable

Edited July 11, 2017 by Explv

*Rekt* · July 11, 2017

55 minutes ago, slazter said:
if (!getBank().isOpen()) {

Went through ur code and i got a suggestion for u when u check for if bank is not open webwalk to bank; add additional check for !myplayer is not in bank (i wrote this in english not in code) dont put this in code :boge:

Juggles · July 11, 2017

Woah woah woah.

This is a big no no

  if (!getBank().isOpen()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
                getBank().open();
            } else if (getBank().isOpen()) {
                getBank().withdraw("Iron bar", 2);
                getBank().withdraw("Redberry pie", 1);
                getBank().withdraw(pickaxe, 1);
            }

You need to check for every action to make sure it is complete. Do it like this instead.
This will work even if an action fails

  if (!getBank().isOpen()) {
if (!BankArea.contains(myPlayer()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
} else {

                getBank().open();
}
            } else if (getBank().isOpen()) {
if (!getInventory.contains("Iron bar") {
                getBank().withdraw("Iron bar", 2);
} else if (!getInventory.contains("Redberry pie") {
                getBank().withdraw("Redberry pie", 1);
} else if (!getInventory.contains(pickaxe) {
                getBank().withdraw(pickaxe, 1);
}
            }

*Rekt* · July 11, 2017

20 minutes ago, Juggles said:

Woah woah woah.

This is a big no no


  if (!getBank().isOpen()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
                getBank().open();
            } else if (getBank().isOpen()) {
                getBank().withdraw("Iron bar", 2);
                getBank().withdraw("Redberry pie", 1);
                getBank().withdraw(pickaxe, 1);
            }

You need to check for every action to make sure it is complete. Do it like this instead.
This will work even if an action fails


  if (!getBank().isOpen()) {
if (!BankArea.contains(myPlayer()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
} else {

                getBank().open();
}
            } else if (getBank().isOpen()) {
if (!getInventory.contains("Iron bar") {
                getBank().withdraw("Iron bar", 2);
} else if (!getInventory.contains("Redberry pie") {
                getBank().withdraw("Redberry pie", 1);
} else if (!getInventory.contains(pickaxe) {
                getBank().withdraw(pickaxe, 1);
}
            }

thats precisely what i meant juggles

Eagle Scripts · July 11, 2017

13 minutes ago, Get Rekt said:

thats precisely what i meant juggles

Only he gave an example so the user could actually understand it aye :boge:

*phony* · July 11, 2017

1 hour ago, Get Rekt said:

Went through ur code and i got a suggestion for u when u check for if bank is not open webwalk to bank; add additional check for !myplayer is not in bank (i wrote this in english not in code) dont put this in code

Noone could get help from this..

*Rekt* · July 11, 2017

Gimme a break :feels: i am travelling+ 3 days into scripting least i could see it

slazter · July 11, 2017

2 hours ago, Tom said:
My first impression is that this is pretty messy

I'm about to head off to bed, but one of the first things I see is related to efficiency / poor design
        Area squire_spot = new Area(2980, 3339, 2975, 3344);
        Area thurgo_spot = new Area(3001, 3143, 2995, 3146);
        Area reldo_spot = new Area(3213, 3490, 3207, 3497);
You are creating a new Area every time the method is called, you should initialise them in the onStart or in a constructor, and then just call them by their variable name.
This increases the variables scope, and is better design for what you are doing in my opinion.

http://www.javawithus.com/tutorial/scope-and-lifetime-of-variables
Area squires_spot;

 

@Override

public void onStart(){

    squire_spot = new Area(2980, 3339, 2975, 3344);

}
Thats just a quick example, I'll drop back by tomorrow but I'm sure @Explv will have sorted you out by then.

As for any new scripter, feel free to add me on skype 'tommyhoogstra'
I'll offer you my free advice and opinions, but you need to be able to take constructive criticism well.

Ohh, wow didn't even knew i actually created a new variable each time the method was called, guess this is going to save alot of memory down the lane when creating bigger scripts! Thanks alot!
And yeh im gonna take you up on that skype deal for sure! gimme hell

1 hour ago, Explv said:
A few things I notice that are bad:

You don't need to create your own methods for handling dialogue. There already exists a method getDialoges().completeDialogue(options) that will click continue and select options etc.

Instead of having if statements for checking configs, you could just use a switch, it would be much cleaner:
switch(getConfigs().get(123)) {
  case 0:
    doSomething();
    break;
  case 1:
    doSomethingElse();
    break;
  // etc.
}
In your questKnightsSword method you have a bunch of Areas. Those should be global final variables, as you are never changing them, and you don't want to keep creating new instances every time that method is called.

The above also applies to the pickaxe filter, val and qItems variables in your questKnightsSword method

In the questKnightsSword method, at the start, you always search for three NPCs. You should only search for an NPC if you are going to use it, e.g. only get the squire npc when you are going to talk to the squire.

Some of your variables are poorly named, such as "val". Use proper naming conventions

You should check the return status of an API call. For example you interact with the blurite rock and then sleep, but you don't actually check if the interaction was successful. If the interaction failed for some weird reason, the script would just sleep for 5 seconds:
bluerite.interact("Mine");
Sleep.sleepUntil(()-> myPlayer().isAnimating(),5000);
Should be:
if (bluerite.interact("Mine")) {
    Sleep.sleepUntil(()-> myPlayer().isAnimating(), 5000);
}
Similar to the point above, you have a bunch of sequential API calls in your code, assuming that the previous call was successful. API calls CAN fail, and you should account for it. For example when you withdraw "q items" you just withdraw one after the other, not accounting for if the previous one failed and you needing to withdraw again.

dialogueV2 is a terrible name for a method

You have some elses without curly braces which could easily result in you messing something up

You also have a few sleeps that are just sleep(random(int, int)). These should all really be replaced by a ConditionalSleep

Regarding your first question, I would clean up your questKnightsSword method, there's quite a few areas in there that could benefit from being moved out into their own methods.

As for you wanting to learn more about lambdas, you are already making use of them in the Sleep.sleepUntil method. You could also use a lambda expression for the pickaxe filter:
Filter<Item> pickaxe = item -> item.getName().contains("pickaxe");
Because you are making a quest script with a bunch of quests, you should focus on studying OOP concepts. There are a lot of common things you have to do in quests, so you should be making proper use of inheritance, utility classes / methods etc. to reduce the amount of code you have to write.

Regarding your 6th point "So i have a class called "Platser" where i save many areas or positions around runescape", I think you should store areas and positions in the places where you are going to use them. The only reason to create a class with variables like that is if you are using them in multiple places, for example the Banks class in the OSBot API.

Damn, I have become too predictable

This is really a goldmine of info! As a new coder i can't thank you enough for all this input, this is really really great!!
I can't really comment on each of your suggestions as of yet, as im gonna rewrite the entire script with your inputs in mind ASAP, and i think i have to work through each of your given concepts to fully understand on how to implement it before i can ask further. I'll start working on this ASAP and i'll post updates if i get stuck, and an updated version of the script in case i don't to see if i understood what was said correctly

1 hour ago, Get Rekt said:

Went through ur code and i got a suggestion for u when u check for if bank is not open webwalk to bank; add additional check for !myplayer is not in bank (i wrote this in english not in code) dont put this in code

1 hour ago, Juggles said:

Woah woah woah.

This is a big no no


  if (!getBank().isOpen()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
                getBank().open();
            } else if (getBank().isOpen()) {
                getBank().withdraw("Iron bar", 2);
                getBank().withdraw("Redberry pie", 1);
                getBank().withdraw(pickaxe, 1);
            }

You need to check for every action to make sure it is complete. Do it like this instead.
This will work even if an action fails


  if (!getBank().isOpen()) {
if (!BankArea.contains(myPlayer()) {
                getWalking().webWalk(Banks.FALADOR_WEST);
} else {

                getBank().open();
}
            } else if (getBank().isOpen()) {
if (!getInventory.contains("Iron bar") {
                getBank().withdraw("Iron bar", 2);
} else if (!getInventory.contains("Redberry pie") {
                getBank().withdraw("Redberry pie", 1);
} else if (!getInventory.contains(pickaxe) {
                getBank().withdraw(pickaxe, 1);
}
            }

Okay great, something new learnt! I thought that the script would automaticly take care of that since i nest everything inside of a if(!Inventory.contains(qitems)), that i thought would reloop if it didn't take those items out correctly, But since everyone of you pointed it out, it seems to be the right choice, point taken! Thanks guys!

I'm gonna start reworking the script ASAP and post an update when im done or stuck.

Edited July 11, 2017 by slazter

Juggles · July 11, 2017

13 minutes ago, slazter said:

Ohh, wow didn't even knew i actually created a new variable each time the method was called, guess this is going to save alot of memory down the lane when creating bigger scripts! Thanks alot!
And yeh im gonna take you up on that skype deal for sure! gimme hell

This is really a goldmine of info! As a new coder i can't thank you enough for all this input, this is really really great!!
I can't really comment on each of your suggestions as of yet, as im gonna rewrite the entire script with your inputs in mind ASAP, and i think i have to work through each of your given concepts to fully understand on how to implement it before i can ask further. I'll start working on this ASAP and i'll post updates if i get stuck, and an updated version of the script in case i don't to see if i understood what was said correctly

Okay great, something new learnt! I thought that the script would automaticly take care of that since i nest everything inside of a if(!Inventory.contains(qitems)), that i thought would reloop if it didn't take those items out correctly, But since everyone of you pointed it out, it seems to be the right choice, point taken! Thanks guys!

I'm gonna start reworking the script ASAP and post an update when im done or stuck.

It will reloop but it can lead to issues :P

Alek · July 13, 2017

Smaller code does not mean more efficient code. Such as the case with lambdas, it doesnt always mean its more efficient.

Sign In

Help me improve my script, and use Lambdas?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Recently Browsing 0 members