Jump to content
×
×
  • Create New...

[On Hold] Script Writer Application


Recommended Posts

6 hours ago, lostmymind891274 said:

Hello,

I would like to apply for the Script Writer I role using my climbing boot collector script. Regardless of the outcome, I'd appreciate tips on my logic / code that can improve performance, etc.

https://github.com/PuppyLover101/powClimbingBoots

Thanks,

Pup

Why all the classes? Way to many imo

Link to post
Share on other sites

  • Moderators
2 hours ago, cyanic said:

Why all the classes? Way to many imo

Keep it constructive..

 

Anyway lets get into it; appologies if this is rather lengthy, figured the more info I can give, the more useful it'll be

While you do have a lot of tasks, the point modularity suggests that each task has 1 purpose. While this can be taken to an extreme level like you have or a somewhat more broad level. Splitting the task of banking into 5 parts is likely going to make it harder on you to maintain in the future which is the opposite purpose of modularity as you have more validation conditions to maintain.

I'm a little confused as to some of the handling you've got in here I'll be honest. I'm assuming you use the webwalker to teleport to ferox?  Movement.walkTo(Constants.FEROX_TILE); However you have 5 tasks for banking but yet your travel to and stat restore is all inside 1 task for this? and then in your GoToBank you .interact() with castlewars on the ring. Doesn't seem consistent and is kinda hard to follow how you're doing it all. So a consistent approach should really be used.

--------------------------------------------------------------------

Lets talk about banking...

We'll start with open bank task.


https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/OpenBank.java#L24
https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/OpenBank.java#L28
On lines 24 & 28 you're passing some of the same variables. Rather than have to check !Main.restockGames && (Game.tab() == Game.Tab.EQUIPMENT) && !Bank.opened() twice, you should look to reduce unnecessary validation and go for something like this

if((!Main.restockGames || !Main.restockDueling) && (Game.tab() == Game.Tab.EQUIPMENT) && !Bank.opened()){
	if(!Equipment.itemAt(Equipment.Slot.RING).name().startsWith("Ring of dueling")){
		System.out.println("Restocking dueling! Doesn't exist!! | " + Equipment.itemAt(Equipment.Slot.RING).name());
        Main.restockDueling = true;
	}
	if (!Equipment.itemAt(Equipment.Slot.NECK).name().startsWith("Games"){
		System.out.println("Restocking games necklace! Doesn't exist!! | " + Equipment.itemAt(Equipment.Slot.NECK).name());
        Main.restockGames = true;
	}
}

So here, you're using the same 3 checks of restock games/dueling game tab and bank opened just the once.

---------------------------------------------

Withdrawing Coins

https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/BankCoins.java#L24

Firstly your coins in bank item is being pulled from the Inventory stream, not the bank stream, so this task is going to be doing nothing. For it to activate, coins aren't valid() in your inventory however you're then asking it to interact with a NIL item because it doesn't exist in your inventory. Instead you should use the Bank APIand do something like this:

Bank.withdraw(995, Bank.Amount.ALL);

And also, you're passing a CLIMBING_BOOTS variable as your ID for coins?

-----------------------------------------------------

Depositing items

https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/BankBoots.java#L33

What's going on here?
It's completely devoid of any Bank API; not sure what your comment about deposit all except was about as if it returns properly, then use it.
Needs tidying up with proper use of the API.

--------------------------------------------------

Bank for dueling ring & game necklace

Again, this is completely devoid of any Bank APIother than Bank.stream(). Needs tidying up with proper use of the API.

However ignoring that; you'll only ever withdraw (8)'s; would be nice to see an item collection and sort to withdraw the largest charge available rather than only (8)'s.

Also, the duel ring and games necklace are almost the exact same code; ideally you could generify that to only have 1 place to fix it. If you update dueling task to use the API, you're then having to duplicate your time and do it for games necks too.

----------------------------

Just something else i saw;

https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/GoToTenzing.java#L33

no need to pass true to interact(); it's true by default, you only need to pass false where you don't want it to use the menu.

----------------------------------

use of else if is potentially slowing you down; https://github.com/PuppyLover101/powClimbingBoots/blob/3765aa4e9e8abb0ba2e389409216fcaa14de8806/src/GrabBoots.java#L34

due to using else if, it's not sequential, so you cant' complete step 1 and then move onto step 2 because it's elsed; meaning you have to wait for the task to finish and then loop back around to this task. With the number of tasks and validations you have per task, this can potentially be a resource and time hog.

----------------------------------

SLEEPS!

Poor use of sleeps; I hate with an absolute passion when people use stuff like Condition.sleep(Random.nextGaussian(420, 590, 94)); when there are better alternatives. 

lets go through why! 

Firstly, your variation is whack.

here's a typical bell curve (what you'd ideally be aiming for)

 image.png

VS what you have

image.png

 

Secondly, that sleep has absolutely no validation in it whatsoever, you'll be sleeping at a minimum 450ms, when you might not need to, you probably won't even need to sleep 200; you could also need to sleep longer, lets say you lag out and you actually needed 2 seconds to catch up, then your 590ms won't be enough.

You use conditional waits throughout the rest of your script but not here? I'm sure you know how to use them but in case it helps, here's something I wrote on them https://docs.powbot.org/#/Fundamentals_In_Practice/UsingWaits

------------------------------------------------

Summary

You've shown an understanding of java within the script but to move forward with this I've listed some points below.

  • Script needs tidying up, varying travel logic is rather messy and hard to follow
    • follow a consistent approach
  • Banking needs severe work; 
    • merge into 1 banking task
    • use banking api
    • optional but would be good to see a collect/sort for withdrawing the highest charge on jewllery
  • Swap out static sleeps for conditional sleeps 
  • Script doesn't/won't work from what I've seen as the withdraw coins task is completely borked.
  • Script seems rushed and un-optimised
    • try to reduce dupe code; this will be easier once you merge your bank tasks.

For now, I've put this on hold; anyone else is free to comment with feedback and let me know when you want me to review the application again, it's a good start.

  • Like 1
  • Thanks 1
Link to post
Share on other sites

  • Dan changed the title to [On Hold] Script Writer Application

@Dan provided some good feedback that should keep you busy for a while, but I'll tack on some additional points as well:

 

Using Tasks

First thing I noticed is how complicated your task's activate() conditionals are. They are hard to understand because there is so many conditions to consider, thus causing it to be difficult to understand at what point in the script that task is going to actually activate and execute. Additionally, this causes unnecessary checks to be included in your activate() methods that have already been or should have been checked in previous tasks. 

This is a common trap I see other scripters fall into when using tasks, but it can be avoided. These are a few things to consider and follow when dealing with tasks:

1. Each task should be responsible for doing only one "thing"

It is easy to find yourself giving a task multiple responsibilities. For example, you might create a FightMonster() task thinking that all it will have to do is attack a monster, but end up including logic for eating food, drinking potions, and equipping armour. All of these additional responsibilities are reflected in both your execute(), but most importantly in your validate(). You might go from your validate being...

    @Override
    public boolean activate() {
        return Npcs.stream().name("Monster").first().valid();
    }

... to ...

    @Override
    public boolean activate() {
        return Npcs.stream().name("Monster").first().valid()
                || (!Players.local().isStaminaActive() && Inventory.stream().name("Potion").first().valid())
                || (!Equipment.stream().name("Shield").first().valid() && Inventory.stream().name("Shield").first().valid())
                || (!Equipment.stream().name("Sword").first().valid() && Inventory.stream().name("Sword").first().valid());
    }

See how out of hand our activate() got? It went from easily readable to a jumbled mess. I use this example to push the importance of splitting up everything into simple tasks. In the previous example, this would mean creating the following tasks: AttackMonster(), DrinkStaminaPot(), EquipArmour(), EquipWeapons().

This whole idea goes back to a basic OOP concept called cohesion. Low cohesion mean a class is doing a lot of things (bad), and high cohesion means the class is doing only one specific thing (good). See the following to get a quick explanation: https://stackoverflow.com/questions/3085285/difference-between-cohesion-and-coupling

 

2.  Your tasks should be ordered from highest priority to lowest priority 

Every loop, your task's validate() methods are executed one by one, starting from the first task you add to your taskList. The order in which you add your tasks is important to consider because conditions that have been checked in previous tasks and are validated first don't have to be checked again in subsequent tasks. This idea is likely better understood with an example:

Say you have 4 tasks, EatFood, IdleWhileInCombat, LootFeathers, and AttackChicken. The first step before even writing code for your tasks is to figure out which task has highest priority. In this case, EatFood seems to be the highest priority task. Even if we are in combat, we want to prioritize eating so that we don't die. 

Following this same logic, we can figure out what our next high priority task should be. In this case, it will be IdleWhileInCombat. Why? Well, if we don't need to eat, we will either be looking for a chicken to attack or feathers to loot. But, what if are already in combat? Do we want to look for a chicken to attack or feathers to loot? No. If we are in combat, we want to idle, so we will put our IdleWhileInCombat task next in the priority list.

Finishing up, we have two tasks left, LootFeathers and AttackChicken. What should have higher priority? Well, since feathers eventually despawn and/or can be looted by other players, we probably want to prioritize looting feathers before attacking a chicken. So, our final code for ordering tasks will be:

...
taskList.add(new EatFood());
taskList.add(new IdleWhileInCombat()));
taskList.add(new LootFeathers());
taskList.add(new AttackChicken());
...

 

3. Keep your activate() method checks as simple as possible

If you have followed step 1 and 2, your activate() methods should typically only contain one or two conditions to check. Reiterating a concept I mentioned in step 2, conditions checked in higher priority tasks don't have to be checked again in subsequent tasks, since you can already infer what the results of those conditions are. For example, referring to the example in step 2, if we check if we have low health in our EatFood activate() we don't need to check if we have low health in our IdleWhileInCombat, LootFeathers, or AttackChicken tasks, since our EatFood activate() checks for this already and is ran before our other tasks. 

Because of this, we can usually keep our activate() checks very simple. For example, in IdleWhileInCombat, we just have to check if we have a current target that is a chicken. In LootFeathers, we just have to check and see if there are feathers on the ground that are close to us. In AttackChicken, we just have to check and see if there is a chicken near us that is not in combat with something else. 

Pretty nice, right?

 

Sleeps

Your sleep that you run at the end of each loop can be included in your poll() method in Main.java instead of having to clutter you tasks with sleeps after every action. This will effectively do the same thing and save you a lot of lines of code:

    @Override
    public void poll() {
        for (Task task : taskList) {
            if (task.activate()) {
                task.execute();
                if (ScriptManager.INSTANCE.isStopping()) {
                    break;
                }
                Condition.sleep(Random.nextInt(200, 800));
              	return;
            }
        }
    }

Edit: I also added a return after a task executes, since this is the typical structure scripters follow when using TaskScript, and the suggestions above assume you are only executing one task per poll() (i.e. loop). 

 

Guard Clauses

Also, look into "guard clauses" to make your code simpler and more readible. https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html

For example your GoToTenzing.java execute() method can be refactored from what you have now...

    @Override
    public void execute() {
        Main.CURRENT_TASK = "GoToTenzing";
        if (Bank.opened()) {
            Bank.close();
            Condition.wait(() -> !Bank.opened(), 150, 50);
        } else {
            if (!Constants.BURTHOPE_AREA.contains(Players.local())) {
                if (Equipment.itemAt(Equipment.Slot.NECK).name().startsWith("Games")) {
                    if (Equipment.itemAt(Equipment.Slot.NECK).interact("Burthorpe", true)) {
                        System.out.println("Teleporting to Burthope");
                        Condition.wait(() -> Constants.BURTHOPE_TILE.distanceTo(Players.local()) < 30, 150, 50);
                    } else {
                        System.out.println("Error when teleporting to Burthope");
                    }
                } else {
                    System.out.println("PROBLEM with interacting with Games Necklace");
                }
            } else {
                Movement.walkTo(Constants.TENZING_TILE);
            }
        }
    }

... to this (using guard clauses):

    @Override
    public void execute() {
        Main.CURRENT_TASK = "GoToTenzing";
        if (Bank.opened()) {
            Bank.close();
            return;
        }
        if (Constants.BURTHOPE_AREA.contains(Players.local())) {
            Movement.walkTo(Constants.TENZING_TILE);
            return;
        }
        if (!Equipment.itemAt(Equipment.Slot.NECK).name().startsWith("Games")) {
            System.out.println("No Games necklace");
            return;
        }
        if (Equipment.itemAt(Equipment.Slot.NECK).interact("Burthorpe", true)) {
            System.out.println("Teleporting to Burthope");
            return;
        }
        System.out.println("Error when teleporting to Burthope");
    }

Ah, much better, right? Now when we read this code, we only have to consider one condition for each action that is performed. Following this structure will make both you and the reader's lives much easier.

 

Summary

So in summary:

  • Follow @Dan's suggestions
  • Follow best practices mentioned in this post for using Tasks
  • Look into OOP concepts, specifically Cohesion and Coupling
  • Simplify sleeps
  • Use guard clauses

I think if you spend some time on these topics and refactor your project, you'll be in a much better position for consideration, and you should have some good concepts under your belt to apply to other projects moving forward. 

Best of luck!

- Fork

  • Like 2
Link to post
Share on other sites

Please sign in to comment

You will be able to leave a comment after signing in



Sign In Now
 Share