Code review my ~40 classes Item Combination Script

[yfoo]

This script is something I worked on over my last summer vacation before starting my actual programming job. It is an item combination script aimed at goldfarming unfinished potions. For the most part it works really well. Overnight runs and so on... with special code to handle breaks. I even used it to get 99 fletching. 

https://imgur.com/qY9sxrx

This is what the paint looks like

https://imgur.com/z9F8Bhr

https://imgur.com/ipOmJ6w

https://imgur.com/cOCn4xG

It uses my fancy crap algorithm to keep doing the same unfinished potions recipe (AVAN, TDFX, ect.) until the price margin for that recipe has fallen below a (HARD_CODED_INTEGER) OR (a previously found cached price margin of a competing recipe). Price margins are found by the insta buy and sell prices of an unfinished potion and its corresponding herb. I use a sell price of 1 and buy price of 5000 to force a buy/sell. 

So because of the above, if its not obvious, the script knows how to buy herbs and sell unfinished potions. 

The undeterministic nature of GE transactions is what I had so many issues over. For the most part my code will handle it, with minor hiccups, but it will correct itself in time. The transaction code is what I'm concerned about, basically I think its a fucking mess. A working mess but not one im happy with. 

If you're willing to look over my code PM me and I may give you source, i been commenting it as I was writing it so hopefully you don't get too lost.  And of course, THANK YOU!' It is hosted as a private github repo so PM me your github.

Im not kidding about the 40 classes thing: https://imgur.com/pGaSTJ4

Trusted please. I been around here long enough to think I know who are reasonable, or have written something I think is cool. <-WTF english
@Alek @Chris @liverare @Juggles @ThatGamerBlue @Fruity @Team_Cape @Explv @Dreamo?  @Im_going_by_memory_here_because_forum_is not_autocompleting_for_me.  @lemons? @Team Cape @Frostbus?     @apeac?  

If you want to look at it be warned, it can get messy. heres a primer:

package ScriptClasses;

import Nodes.BankingNodes.DecideRestockNode;
import Nodes.BankingNodes.DepositNode;
import Nodes.BankingNodes.OptionalInvFixNode;
import Nodes.BankingNodes.PrimaryWithdraw.Withdraw10Primary;
import Nodes.BankingNodes.PrimaryWithdraw.Withdraw14Primary;
import Nodes.BankingNodes.PrimaryWithdraw.WithdrawXPrimary;
import Nodes.BankingNodes.SecondaryWithdraw.Withdraw10Secondary;
import Nodes.BankingNodes.SecondaryWithdraw.Withdraw14Secondary;
import Nodes.BankingNodes.SecondaryWithdraw.WithdrawXSecondary;
import Nodes.CreationNodes.AFKCreation;
import Nodes.CreationNodes.HoverBankerCreation;
import Nodes.CreationNodes.PrematureStopCreation;
import Nodes.DebuggingNode;
import Nodes.GENodes.*;
import Nodes.MarkovChain.MarkovNodeExecutor;
import Nodes.StartingNode;
import Util.Margins;
import Util.Statics;
import org.osbot.rs07.script.MethodProvider;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

import static ScriptClasses.MainScript.SCRIPT_NAME;

@ScriptManifest(author = "PayPalMeRSGP", name = SCRIPT_NAME, info = "item combiner, but mainly used for unf potions", version = 0.5, logo = "")
public class MainScript extends Script {
    static final String SCRIPT_NAME = "Item_Combinator";

    private MarkovNodeExecutor executor;

    private DebuggingNode debug;
    private boolean runDebugNode = false;

    @Override
    public void onStart() throws InterruptedException {
        super.onStart();
        Statics.script = this;
        markovChainSetup();
        camera.movePitch(67);
        new ScriptPaint(this);
    }

    @Override
    public int onLoop() throws InterruptedException {
        return executor.executeThenTraverse();
    }

    private void markovChainSetup(){
        StartingNode start = new StartingNode(this);
        Withdraw10Primary w10P = new Withdraw10Primary(this);
        Withdraw14Primary w14P = new Withdraw14Primary(this);
        WithdrawXPrimary wXP = new WithdrawXPrimary(this);

        Withdraw10Secondary w10S = new Withdraw10Secondary(this);
        Withdraw14Secondary w14S = new Withdraw14Secondary(this);
        WithdrawXSecondary wXS = new WithdrawXSecondary(this);

        DecideRestockNode restock = new DecideRestockNode(this);
        DepositNode deposit = new DepositNode(this);
        OptionalInvFixNode fix = new OptionalInvFixNode(this);

        AFKCreation afk = new AFKCreation(this);
        HoverBankerCreation hover = new HoverBankerCreation(this);
        PrematureStopCreation premature = new PrematureStopCreation(this);

        AbortRelevantOffers abort = new AbortRelevantOffers(this);
        GESpinLockBuyNode buy = new GESpinLockBuyNode(this);
        GESpinLockSellNode sell = new GESpinLockSellNode(this);
        IntermittentBuy randBuy = new IntermittentBuy(this);
        IntermittentSell randSell = new IntermittentSell(this);
        InitialBuy initialBuy = new InitialBuy(this);

        executor = new MarkovNodeExecutor(start, w10P, w14P, wXP, w10S, w14S, wXS, restock, deposit, fix, afk, hover, premature, buy, sell, randBuy, randSell, initialBuy, abort);
    }

    @Override
    public void onExit() throws InterruptedException {
        super.onExit();
        Margins.markSingletonAsNull();
        ScriptPaint.geOpsEnabled = true;
    }
}

 

Special Notes:

Margins.java - logic for GE transactions here

StartNode.java - first node to be executed, and only executed once, has logic to connect to various other nodes based on state of inventory + bank.

Nodes then execute in order in accordance to: 

https://imgur.com/CtAP0Tl

^above is out of date but gives a good enough understanding

See getAdjacentNodes() for the list of successor nodes

Edited September 16, 2018 by PayPalMeRSGP

[liverare]

I can't say no to a challenge.

 

[yfoo]

Its in a private github repository, so PM me github linked emails please. 

[yfoo]

Update1:

@liverare helped me alot with code readability, namely...

• Improve the readability of CombinationRecipe.java enum by creating a seperate ItemData enum that CombinationRecipe uses. Origionally CombinationRecipe.java looked like this...

public enum CombinationRecipes {

    AVANTOE("Avantoe", "Vial of water", "Avantoe potion (unf)", "avan", 261, 227, 103, 50, true),
    TOADFLAX("Toadflax", "Vial of water", "Toadflax potion (unf)", "toadfl", 2998, 227, 3002, 34, true),
    RANARR("Ranarr", "Vial of water", "Ranarr potion (unf)", "ranar", 257, 227, 99, 30, true),
    IRIT("Irit leaf", "Vial of water", "Irit potion (unf)", "irit", 259, 227, 101, 45, true),
    KWUARM("Kwuarm", "Vial of water", "Kwuarm potion (unf)", "kwu", 263, 227, 105, 55, true),
    HARRALANDER("Harralander", "Vial of water", "Harralander potion (unf)", "harra", 255, 227, 97, 21, true),

    ATTACK_POTION("Guam potion (unf)", "Eye of newt", "Attack potion (3)", "n/a", 91, 221, 121, 3, false),

    CLAY("Clay", "Jug of water", "Soft clay", "clay", 434, 1937, 1761, 1, false),
    AIR_BATTLESTAFF("Battlestaff", "Air orb", "Air Battlestaff", "", 1391, 573, 1397, 66, false),
    YEW_LONGBOW("Yew longbow (u)", "Bow string", "Yew longbow", "yew long", 66, 1777, 855, 70, false);
	... variables, constructor, and getter methods
}

multiple fields such as the item id for vial of water (227) were repeated in every entry. This is a problem because suppouse an item id were to update; every entry would need to be changed. It is also not clear what the 4 int values in each enum were, and I wrote this. ItemData comes into play here by defining specific mappings of itemName to ItemID that my refractored CombinationRecipe uses.

ItemData.java:

package Util;

import org.osbot.rs07.api.filter.Filter;
import org.osbot.rs07.api.model.Item;

//Credits to LiveRare
public enum ItemData implements Filter<Item> {
    VIAL_OF_WATER("Vial of water", 227),

    CLEAN_AVANTOE("Avantoe", 261),
    CLEAN_TOADFLAX("Toadflax", 2998),
    CLEAN_IRIT("Irit leaf", 259),
    CLEAN_KWUARM("Kwuarm", 263),
    CLEAN_HARRALANDER("Harralander", 255),

    UNF_AVANTOE_POTION("Avantoe potion (unf)", 103),
    UNF_TOADFLAX_POTION("Toadflax potion (unf)", 3002),
    UNF_IRIT_POTION("Irit potion (unf)", 101),
    UNF_KWUARM_POTION("Kwuarm potion (unf)", 105),
    UNF_HARRALANDER_POTION("Harralander potion (unf)", 97);

    private String name;
    private int id;

    ItemData(String name, int id) {
        this.name = name;
        this.id = id;
    }

    @Override
    public String toString() {
        return String.format("[%s] %s", id, name);
    }

    public String getName() {
        return name;
    }

    public int getId() {
        return id;
    }

    @Override
    public boolean match(Item item) {
        return item.getId() == id || item.getName().equals(name);
    }
}

And CombinationRecipe.java now uses this enum to define recipes like so:

package Util;
import org.osbot.rs07.api.ui.Skill;

public enum CombinationRecipes {
    AVANTOE_UNF_RECIPE(ItemData.CLEAN_AVANTOE, ItemData.VIAL_OF_WATER, ItemData.UNF_AVANTOE_POTION, Skill.HERBLORE, 50, true),
    TOADFLAX_UNF_RECIPE(ItemData.CLEAN_TOADFLAX, ItemData.VIAL_OF_WATER, ItemData.UNF_TOADFLAX_POTION, Skill.HERBLORE, 34, true),
    IRIT_UNF_RECIPE(ItemData.CLEAN_IRIT, ItemData.VIAL_OF_WATER, ItemData.UNF_IRIT_POTION, Skill.HERBLORE, 48, true),
    KWUARM_UNF_RECIPE(ItemData.CLEAN_KWUARM, ItemData.VIAL_OF_WATER, ItemData.UNF_KWUARM_POTION, Skill.HERBLORE, 55, true),
    HARRALANDER_UNF_RECIPE(ItemData.CLEAN_HARRALANDER, ItemData.VIAL_OF_WATER, ItemData.UNF_HARRALANDER_POTION, Skill.HERBLORE, 22, true);

    private ItemData primary, secondary, product;
    private Skill skill;
    private int reqLvl;
    private boolean canUseGE;

    CombinationRecipes(ItemData primary, ItemData secondary, ItemData product, Skill skill, int reqLvl, boolean canUseGE) {
        this.primary = primary;
        this.secondary = secondary;
        this.product = product;
        this.skill = skill;
        this.reqLvl = reqLvl
        this.canUseGE = canUseGE;
    }

    public ItemData getPrimary() {
        return primary;
    }

    public ItemData getSecondary() {
        return secondary;
    }

    public ItemData getProduct() {
        return product;
    }

    public Skill getSkill() {
        return skill;
    }

    public int getReqLvl() {
        return reqLvl;
    }

    public String getGESeachTerm() {
        return primary.getName().trim().split(" ")[0];
    }

    public boolean isCanUseGE() {
        return canUseGE;
    }
}

The same information in the old CombinationRecipe.java is still there however now the composition of each enum value is alot more readable. Another advantage is the implementation of Filter<Item> in ItemData.java. The ItemContainer class (extended by Bank and Inventory) defines overloaded methods for contains, interact, getAmount, etc that besides taking in an item name or item ID can also take in a class defining the Filter<Item> interface. This allows me to simply write code like...

inventory.contains(recipe.getPrimary())
bank.withdraw(recipe.getPrimary()), bank.withdraw(recipe.getSecondary()) and bank.deposit(recipe.getProduct())
  
  

Because each node class holds a reference to the current recipe (so all nodes know what to withdraw, interact, etc) alot of refractoring work needs to go into each class using CombinationRecipe.java. 

Furthermore as hinted in the MainScript.java file, every node object is passed an instance of script because for example, Withdraw nodes need API access for banking. A problem with passing script is to access api methods you need to call script.getXXX().doSomething(), or Bank bank = script.getBank() and bank.withdraw(something). Instead it is alot cleaner to have each Node extend MethodProvider and exchangeContext with a passed bot instance. That way API access code does not require a call to getXXX().  

IMO I think documentation for providing API access to your classes could use some work. Especially since exchangeContext is marked as deprecated which inadvertantly encourages scriptors to pass script around. 

There is alot to change around and I won't commit anything until I can confirm that all refractored code runs. 

Anyway, thank you, that is all. 

Edited September 19, 2018 by PayPalMeRSGP