Grand Exchange feedback

[MarWo22]

Hello

This is one of the first scripts i have ever made. This is a script for buying the best margin herbs in the GE. There are probably a lot of things I can improve on. Can you guys please give me some feadback? 

Thank you

 

The script:

Spoiler

 

package tasks;

import java.io.IOException;

import org.osbot.rs07.api.Bank;
import org.osbot.rs07.api.Bank.BankMode;
import org.osbot.rs07.api.model.NPC;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.utility.ConditionalSleep;

import utils.GrandExchange;

public class GrandExchangeTask {
    
    String highestMarginHerbName;
    int highestMarginHerbId;
    int highestMarginHerbBuyingPrice;
    
    
    

    public GrandExchangeTask(Script sI) throws IOException, InterruptedException {
        
        final GrandExchange ge = new GrandExchange();
        
        int buyingPriceIritHerb = ge.getBuyingPrice(259);
        int sellingPriceIritHerb = ge.getSellingPrice(259);
        int buyingPriceIritUnf = ge.getBuyingPrice(101);
        int sellingPriceIritUnf = ge.getSellingPrice(101);
        int priceIrit = Math.max(buyingPriceIritHerb, sellingPriceIritHerb);
        int priceIritUnf = Math.min(buyingPriceIritUnf, sellingPriceIritUnf);
        int iritMargin = priceIritUnf - priceIrit;
        
        int buyingPriceToadflaxHerb = ge.getBuyingPrice(2998);
        int sellingPriceToadflaxHerb = ge.getSellingPrice(2998);
        int buyingPriceToadflaxUnf = ge.getBuyingPrice(3002);
        int sellingPriceToadflaxUnf = ge.getSellingPrice(3002);
        int priceToadflax = Math.max(buyingPriceToadflaxHerb, sellingPriceToadflaxHerb);
        int priceToadflaxUnf = Math.min(buyingPriceToadflaxUnf, sellingPriceToadflaxUnf);
        int toadflaxMargin = priceToadflaxUnf - priceToadflax;
        
        int buyingPriceAvantoeHerb = ge.getBuyingPrice(261);
        int sellingPriceAvantoeHerb = ge.getSellingPrice(261);
        int buyingPriceAvantoeUnf = ge.getBuyingPrice(103);
        int sellingPriceAvantoeUnf = ge.getSellingPrice(103);
        int priceAvantoe = Math.max(buyingPriceAvantoeHerb, sellingPriceAvantoeHerb);
        int priceAvantoeUnf = Math.min(buyingPriceAvantoeUnf, sellingPriceAvantoeUnf);
        int avantoeMargin = priceAvantoeUnf - priceAvantoe;
        
        int buyingPriceKwuarmHerb = ge.getBuyingPrice(263);
        int sellingPriceKwuarmHerb = ge.getSellingPrice(263);
        int buyingPriceKwuarmUnf = ge.getBuyingPrice(105);
        int sellingPriceKwuarmUnf = ge.getSellingPrice(105);
        int priceKwuarm = Math.max(buyingPriceKwuarmHerb, sellingPriceKwuarmHerb);
        int priceKwuarmUnf = Math.min(buyingPriceKwuarmUnf, sellingPriceKwuarmUnf);
        int kwuarmMargin = priceKwuarmUnf - priceKwuarm;
        
        int buyingPriceCadantineHerb = ge.getBuyingPrice(265);
        int sellingPriceCadantineHerb = ge.getSellingPrice(265);
        int buyingPriceCadantineUnf = ge.getBuyingPrice(107);
        int sellingPriceCadantineUnf = ge.getSellingPrice(107);
        int priceCadantine = Math.max(buyingPriceCadantineHerb, sellingPriceCadantineHerb);
        int priceCadantineUnf = Math.min(buyingPriceCadantineUnf, sellingPriceCadantineUnf);
        int cadantineMargin = priceCadantineUnf - priceCadantine;
         
        int buyingPriceLantadymeHerb = ge.getBuyingPrice(2481);
        int sellingPriceLantadymeHerb = ge.getSellingPrice(2481);
        int buyingPriceLantadymeUnf = ge.getBuyingPrice(2483);
        int sellingPriceLantadymeUnf = ge.getSellingPrice(2483);
        int priceLantadyme = Math.max(buyingPriceLantadymeHerb, sellingPriceLantadymeHerb);
        int priceLantadymeUnf = Math.min(buyingPriceLantadymeUnf, sellingPriceLantadymeUnf);
        int lantadymeMargin = priceLantadymeUnf - priceLantadyme;
        
        int highestMargin = Math.max(iritMargin, Math.max(toadflaxMargin, Math.max(avantoeMargin, Math.max(kwuarmMargin, Math.max(cadantineMargin, lantadymeMargin)))));
        
        NPC clerk = sI.getNpcs().closest("Grand Exchange Clerk");
        
        if(highestMargin == iritMargin) {
            highestMarginHerbName = "Irit leaf";
            highestMarginHerbId = 101;
            highestMarginHerbBuyingPrice = priceIrit;
        } else if(highestMargin == toadflaxMargin) {
            highestMarginHerbName = "Toadfalx";
            highestMarginHerbId = 2998;
            highestMarginHerbBuyingPrice = priceToadflax;
        } else if(highestMargin == avantoeMargin) {
            highestMarginHerbName = "Avantoe";    
            highestMarginHerbId = 261;
            highestMarginHerbBuyingPrice = priceAvantoe;
        } else if(highestMargin == kwuarmMargin) {
            highestMarginHerbId = 263;
            highestMarginHerbName = "Kwuarm";
            highestMarginHerbBuyingPrice = priceKwuarm;
        } else if(highestMargin == cadantineMargin) {
            highestMarginHerbName = "Cadantine";
            highestMarginHerbId = 265;
            highestMarginHerbBuyingPrice = priceCadantine;
        } else {
            highestMarginHerbName = "Lantadyme";
            highestMarginHerbId = 2481;
            highestMarginHerbBuyingPrice = priceLantadyme;
        }
        if(!sI.getBank().isOpen()) {
        if(!sI.grandExchange.isOpen()) {
            sI.getBank().open();
            new ConditionalSleep(10000) {
                
                @Override
                public boolean condition() throws InterruptedException {
                    return sI.getBank().isOpen();
                }
            }.sleep();
        }
        }else {
            
            if(!sI.bank.getWithdrawMode().equals(Bank.BankMode.WITHDRAW_NOTE)) {
                sI.bank.enableMode(BankMode.WITHDRAW_NOTE);
            }
            sI.getBank().withdrawAll("Coins");
            
            if(sI.getBank().contains("Irit potion (unf)")) {
                sI.getBank().withdrawAll("Irit potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().contains("Irit potion (unf");
                    }
                }.sleep();
            }
            
            if(sI.getBank().contains("Toadflax potion (unf)")) {
                sI.getBank().withdrawAll("Toadflax potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().contains("Toadfalx potion (unf");
                    }
                }.sleep();
            }
            
            if(sI.getBank().withdrawAll("Avantoe potion (unf)")){
                sI.getBank().withdrawAll("Avantoe potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().contains("Avantoe potion (unf)");
                    }
                }.sleep();
            }
            
            if(sI.getBank().contains("Kwuarm potion (unf)")) {
                sI.getBank().withdrawAll("Kwuarm potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().contains("Kwuarm potion (unf");
                    }
                }.sleep();
            }
            
            if(sI.getBank().contains("Cadantine potion (unf)")) {
                sI.getBank().withdrawAll("Cadantine potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().contains("Cadantine potion (unf)");
                    }
                }.sleep();
            }
            
            if(sI.getBank().withdrawAll("Lantadyme potion (unf)")) {
                sI.getBank().withdrawAll("Lantadyme potion (unf)");
                new ConditionalSleep(1000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.getBank().withdrawAll("Lantadyme potion (unf)");
                    }
                }.sleep();
            }
            sI.getBank().close();
            
            if(!sI.getBank().isOpen() && !sI.getGrandExchange().isOpen()) {
                if(clerk != null) {
                    clerk.interact("Exchange");
                    new ConditionalSleep(2000) {
                        
                        @Override
                        public boolean condition() throws InterruptedException {
                            return sI.getGrandExchange().isOpen();
                        }
                    }.sleep();
                }
            }
            
            if(sI.getInventory().contains("Irit potion (unf)")) {
                sI.grandExchange.sellItem(102, priceToadflaxUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep();
            }
            
            if(sI.getInventory().contains("Toadflax potion (unf)")) {
                sI.grandExchange.sellItem(3003, priceToadflaxUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        // TODO Auto-generated method stub
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep();
            }
            
            if(sI.getInventory().contains("Avantoe potion (unf)")) {
                sI.grandExchange.sellItem(104, priceToadflaxUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep(); 
                
            }
            
            if(sI.getInventory().contains("Kwuarm potion (unf)")) {
                sI.grandExchange.sellItem(106, priceKwuarmUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep();
            }
            
            if(sI.getInventory().contains("Cadantine potion (unf)")) {
                sI.grandExchange.sellItem(108, priceCadantineUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep();
            }
            
            if(sI.getInventory().contains("Lantadyme potion (unf)")) {
                sI.grandExchange.sellItem(2484, priceLantadymeUnf, 9999);
                new ConditionalSleep(5000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isSellOfferOpen();
                    }
                }.sleep();
            }
            
            if(!sI.grandExchange.isOfferScreenOpen()) {
                sI.grandExchange.buyItem(highestMarginHerbId, highestMarginHerbName, highestMarginHerbBuyingPrice, 10);
                new ConditionalSleep(4000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isOfferScreenOpen();
                    }
                }.sleep();
            } else {
                sI.grandExchange.goBack();
            }
            
            if(!sI.grandExchange.isOfferScreenOpen()) {
                sI.grandExchange.buyItem(227, "Vial", 5, 10);
                new ConditionalSleep(4000) {
                    
                    @Override
                    public boolean condition() throws InterruptedException {
                        return !sI.grandExchange.isOfferScreenOpen();
                    }
                }.sleep();
            } else {
                sI.grandExchange.goBack();
            }
            
            sI.grandExchange.collect();
            
            sI.grandExchange.close();
        }
    }
    
}

And the snippet for price checking:

Spoiler

package utils;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.URL;

/**
 * GrandExchange Price Class
 * 
 * @author Reid
 *
 */
public class GrandExchange {

    private static final String BASE = "https://api.rsbuddy.com/grandExchange?a=guidePrice&i=";

    /**
     * Default Constructor
     * 
     */
    public GrandExchange() {

    }
    
    /**
     * Gets the overall price of an item.
     * 
     * @param itemID
     * @return itemPrice
     * @throws IOException
     */
    public int getOverallPrice(final int itemID) throws IOException {
        return parse(itemID,"overall");
    }

    
    /**
     * Gets the buying price of an item.
     * 
     * @param itemID
     * @return itemPrice
     * @throws IOException
     */
    public int getBuyingPrice(final int itemID) throws IOException {
        return parse(itemID,"buying");
    }
    
    /**
     * Gets the selling price of an item.
     * 
     * @param itemID
     * @return itemPrice
     * @throws IOException
     */
    public int getSellingPrice(final int itemID) throws IOException {
        return parse(itemID,"selling");
    }

    /**
     * Retrieves the price of an item.
     * 
     * @param itemID
     * @return itemPrice
     * @throws IOException
     */
    private int parse(final int itemID, String choice) throws IOException {
        final URL url = new URL(BASE + itemID);
        BufferedReader file = new BufferedReader(new InputStreamReader(url.openStream()));
        String line;
        String price = null;
        while ((line = file.readLine()) != null) {
            if (line.contains("{")) {
                price = (line).trim();
            }
        }
        if (choice.equals("buying")){
            price = price.substring(price.indexOf(",") + 10, nthOccurrence(price, ',', 1)).trim();
        } else if(choice.equals("selling")) {
            price = price.substring(nthOccurrence(price, ',', 2)  + 11 , price.indexOf("sellingQuantity") - 2).trim();
        } else {
            price = price.substring(price.indexOf(":") + 1, price.indexOf(",")).trim();
        }
        file.close();
        return Integer.parseInt(price);
    }

    private int nthOccurrence(String str, char c, int n) {
        int pos = str.indexOf(c, 0);
        while (n-- > 0 && pos != -1)
            pos = str.indexOf(c, pos + 1);
        return pos;
    }

    }

Edited March 26, 2018 by MarWo22

[Eagle Scripts]

A good practice is to make this more generic.

Make an Enum class called Herb. Save properties in there such as it's itemId.

You should then be able to iterate over the Herb#values. In every iteration, check whether that margin is better then the currently best known margin. If so? update the currently best known margin enum object with the new herb one.

I'm currently not able to write you a quick and dirty snippet but I hope you understand what I mean. If you don't, I'll try to write you a snippet in an hour or so.

[MarWo22]

I dont understand it fully. If you would be able to write a quick small example if it would help me  a lot.

 

[battleguard]

You need to learn to use classes for grouping items together. If you use this class throughout your script you will remove all of your redundant code where you have to check each type. (Edit as EagleScript said Enum Classes will make this way better so here is the same thing written with enums now)

 

public enum Potion {
        IRIT(259, 101),
        TOADFLAX(2998, 3002),
        AVENTOE(261, 103),
        KWUARM(263, 105),
        CANDENTINE(265, 107),
        LANTADYME(2481, 2483);

        public final int HerbPrice;
        public final int Margin;
        public final int UnfinishedPrice;
        public final String Name;
        public final int HerbId;
        public final int UnfinishedId;
        private final String UnfinishedName;

        Potion(int herbId, int unfinishedId) {
            Name = name().substring(0,1).toUpperCase() + name().substring(1).toLowerCase();
            UnfinishedName = Name + " potion (unf)";
            HerbId = herbId;
            UnfinishedId = unfinishedId;
            // TODO: you should make your GE lookup class methods static so you dont need to instiantiate this.
            GrandExchange ge = new GrandExchange();
            HerbPrice = Math.max(ge.getBuyingPrice(HerbId), ge.getSellingPrice(HerbId));
            UnfinishedPrice = Math.min(ge.getBuyingPrice(UnfinishedId), ge.getSellingPrice(UnfinishedId));
            Margin = UnfinishedPrice - HerbPrice;
        }
    }

// here is how you would get the current potion you are using
Potion highestMarginPotion = Arrays.stream(Potion.values()).max(Comparator.comparingInt(p -> p.Margin)).get();

Please make sure you know how to use classes though before using this code.

Edited March 26, 2018 by battleguard

[Apaec]

15 minutes ago, battleguard said:

You need to learn to use classes for grouping items together. If you use this class throughout your script you will remove all of your redundant code where you have to check each type

    public class Potion {
        public final int HerbPrice;
        public final int UnfinishedPrice;
        public final String Name;
        public final int HerbId;
        public final int UnfinishedId;

        public Potion(String name, int herbId, int unfinishedId) {
            Name = name;
            HerbId = herbId;
            UnfinishedId = unfinishedId;
            HerbPrice = Math.max(ge.getBuyingPrice(HerbId), ge.getSellingPrice(HerbId));
            UnfinishedPrice = Math.min(ge.getBuyingPrice(UnfinishedId), ge.getSellingPrice(UnfinishedId));
        }

        public int getMargin() {
            return UnfinishedPrice - HerbPrice;
        }

        public String getUnfinishedName()
        {
            return Name + " potion (unf)";
        }
    }

... on your setup
          Potion[] potions =
                {
                        new Potion("Irit", 259, 101),
                        new Potion("Toadflax", 2998, 3002),
                        new Potion("Aventoe", 261, 103),
                        new Potion("Kwuarm", 263, 105),
                        new Potion("cadentine", 265, 107),
                        new Potion("Lantadym", 2481, 2483)
                };
        Potion potion = Arrays.stream(potions).max( Comparator.comparingInt(p -> p.getMargin())).get();

 

Since all the data is constant, may as well store it in an enum, but yeah this looks good

[battleguard]

5 minutes ago, Apaec said:

Since all the data is constant, may as well store it in an enum, but yeah this looks good

Ya I forgot java allows enums to store more than just a single value im used to c# enums. Thanks I fixed above code.

Edited March 26, 2018 by battleguard

[MarWo22]

public enum Potion {
        IRIT(259, 101),
        TOADFLAX(2998, 3002),
        AVENTOE(261, 103),
        KWUARM(263, 105),
        CANDENTINE(265, 107),
        LANTADYME(2481, 2483);

        public final int HerbPrice;
        public final int Margin;
        public final int UnfinishedPrice;
        public final String Name;
        public final int HerbId;
        public final int UnfinishedId;
        private final String UnfinishedName;

        Potion(int herbId, int unfinishedId) {
            Name = name().substring(0,1).toUpperCase() + name().substring(1).toLowerCase();
            UnfinishedName = Name + " potion (unf)";
            HerbId = herbId;
            UnfinishedId = unfinishedId;
            // TODO: you should make your GE lookup class methods static so you dont need to instiantiate this.
            GrandExchange ge = new GrandExchange();
            HerbPrice = Math.max(ge.getBuyingPrice(HerbId), ge.getSellingPrice(HerbId));
            UnfinishedPrice = Math.min(ge.getBuyingPrice(UnfinishedId), ge.getSellingPrice(UnfinishedId));
            Margin = UnfinishedPrice - HerbPrice;
        }
    }

// here is how you would get the current potion you are using
Potion highestMarginPotion = Arrays.stream(Potion.values()).max(Comparator.comparingInt(p -> p.Margin)).get();

 

OK I understand everything so far. But how would I be able to call the price of for example the herb of the highestMarginPotion?

 

[battleguard]

calling the price is as easy as using a field on the Potion class. You might want to look into using classes in a java tutorial if your still having trouble with the above code.

[MarWo22]

Yea im still a beginner. Might be helpfull to learn java properly first