Hey, so I just finished writing my first script for learning purposes. I'd like to get some feedback on ways to refactor this logic down a bit more.

Here's my code:

SPOILER: It basically catches and banks shrimps/anchovies :P

 

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.api.model.Entity;
import org.osbot.rs07.api.map.Position;
import org.osbot.rs07.script.MethodProvider;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;

import java.util.function.BooleanSupplier;

@ScriptManifest(author = "Jake Slang", name = "JakesShrimpCatcher", info = "Catches shrimp (and anchovies) in Lumbridge swamp.", version = 0.1, logo = "")
public final class JakesShrimpCatcher extends Script  {

    private final Area lumbyFishing = new Area(3240, 3150, 3238, 3145);
    private final Position lumbyBank = new Position(3208, 3220, 2);

    @Override
    public final void onStart() {
        log("Is there anybody actually reading THIS?!");
    }

    @Override
    public final int onLoop() throws InterruptedException {
        if (canFishShrimps()) {
            fish();
        } else {
            bank();
        }

        return random(200, 300);
    }

    @Override
    public final void onExit() {
        log("I guess YOU did!");
    }

    private void fish() {
        if (!lumbyFishing.contains(myPosition())) {
            getWalking().webWalk(lumbyFishing);
        }

        Entity fishingSpot = getNpcs().closest("Fishing spot");

        if (fishingSpot != null && fishingSpot.interact("Net")) {
            sleepConditional();
        }

    }

    private void sleepConditional() {
        Entity fishingSpot = getNpcs().closest("Fishing spot");

        if (fishingSpot != null && fishingSpot.interact("Net")) {
            Sleep.sleepUntil(() -> myPlayer().isAnimating() || !fishingSpot.exists(), 5000);
            log(Sleep.sleepUntil(() -> myPlayer().isAnimating() || !fishingSpot.exists(), 5000));
        }
    }

    private void bank() throws InterruptedException {
        if (getInventory().isFull()) {
            if (!Banks.LUMBRIDGE_UPPER.contains(myPosition())) {
                getWalking().webWalk(lumbyBank);
                MethodProvider.sleep(2000);
            } else if (!getBank().isOpen()) {
                getBank().open();
                getBank().depositAll();
            } else {
                stop(true);
            }
        }
    }

    private boolean canFishShrimps() throws InterruptedException {
        if (!getInventory().contains("Small fishing net")) {
            getWalking().webWalk(lumbyBank);
            MethodProvider.sleep(2000);
            getBank().open();
            getBank().withdraw("Small fishing net", 1);
        }

        return !myPlayer().isAnimating() && !getInventory().isFull();
    }

}

class Sleep extends ConditionalSleep {

    private final BooleanSupplier condition;

    public Sleep(final BooleanSupplier condition, final int timeout) {
        super(timeout);
        this.condition = condition;
    }

    @Override
    public final boolean condition() throws InterruptedException {
        return condition.getAsBoolean();
    }

    public static boolean sleepUntil(final BooleanSupplier condition, final int timeout) {
        return new Sleep(condition, timeout).sleep();
    }

}

Plot twist: It basically catches shrimps/anchovies and banks them :P

Edited July 29, 20178 yr by Jake Slang

You're trying to fix something that doesn't need fixing.

Are you willing to take the risk of introducing bugs in working code simply because you're in the mood to refactor? 

You're done with this code,   go find something harder to write now  

PS: nice logs 

13 minutes ago, Botre said:

You're trying to fix something that doesn't need fixing.

Are you willing to take the risk of introducing bugs in working code simply because you're in the mood to refactor? 

You're done with this code,   go find something harder to write now  

PS: nice logs 

You had me question my own perfectionist tendencies to constantly refactor code, thanks for the thought

 OK, I was just hoping for some remarks on for example my sleep() usage. I think I could do things more efficiently.. If I learn to see these gotchas faster, it'll make me a better programmer overall, really.

Edited July 29, 20178 yr by Jake Slang

Like Botre said, move onto something more advanced and you'll pick up on things.  What I would say is you rely strongly on your logic executing successfully. Try think about your logic flow in your next script, also you can use Banks#bankName rather than grabbing their areas.  

 

21 minutes ago, HeyImJamie said:

Like Botre said, move onto something more advanced and you'll pick up on things.  What I would say is you rely strongly on your logic executing successfully. Try think about your logic flow in your next script, also you can use Banks#bankName rather than grabbing their areas.  

Hey, thanks for replying. Do you have any suggestions? What kind of scripts did you write before applying for Scripter 1?

Yeah, I specifically grabbed the tile position of the Lumbridge bank, because else it would bug around on banking.
 

Edited July 29, 20178 yr by Jake Slang

1 hour ago, Botre said:

You're trying to fix something that doesn't need fixing.

Are you willing to take the risk of introducing bugs in working code simply because you're in the mood to refactor? 

You're done with this code,   go find something harder to write now  

PS: nice logs 

On the contrary, I think that it's important to take a second look and even a third look over your code to improve its performance and possibly find/fix problems you may not have seen the first time. 

 

1 hour ago, Jake Slang said:

 OK, I was just hoping for some remarks on for example my sleep() usage. I think I could do things more efficiently.. If I learn to see these gotchas faster, it'll make me a better programmer overall, really.

I think there are a good number of things you can improve in this script. Such as your sleep usage, overall logic (conditional logic/flow of the program), and variable use.

 

1) In terms of the logic of your script, I feel you need to add quite a few more if statements. The reason for this is to limit the number of actions that you take in one loop iteration of the method onLoop() to one. The method onLoop() is meant to be able to be called many times a second or to be at the rate of about one tick per loop iteration. Heres an example where you could add more if statements.

private boolean canFishShrimps() throws InterruptedException {

        if (!getInventory().contains("Small fishing net")) {
            getWalking().webWalk(lumbyBank);
            MethodProvider.sleep(2000);
            getBank().open();
            getBank().withdraw("Small fishing net", 1);
        }

        return !myPlayer().isAnimating() && !getInventory().isFull();
}

could be broken into something like this:

if(!getInventory().contains("Small fishing net")) {
     if(!lumbyBank.contains(myPlayer().getPosition())) {
          getWalking().webWalk(lumbyBank);
     }
} else { // you are in lumby bank
     if(getBank().isOpen()) {
          if(getBank().contains("Small fishing net")) {
               getBank().withdraw("Small fishing net");
          }
     } else { // bank isn't open
          getBank().open();
     }
}

Other things such as when you choose to go to the bank to get the fishing net could also be addressed. There are a large number of other problems in your logic, but I feel if you break the problem down into smaller steps and write them out in this if/else fashion it will help you fix them.

 

2) The methods fish() and sleepConditional() contain different "fishingSpot" variables. You could create the variable "fishingSpot" in the class scope by putting it after your area and position. Assign it null initially and assign it the closest fishing spot in the first method fish(). The reason for this is that you assign "fishingSpot" the closest fishing spot twice and potentially they could be different.

 

3) I feel another problem is your sleepConditional() method. You have the exact same code in that method as in the fish() method except that you add the sleep, and then log the sleep (lol). By logging the sleep you're actually calling it a second time. You could assign the first Sleep.sleepUntil to a variable and then log that variable or just log one sleep.

 

Edit: Busy for now, but I may come back and change/add more stuff to this list to make it more complete and concise.

Edit 2: On second thought you could move on to a new script and just apply some of these things to that.

Edited July 29, 20178 yr by TheWind

Thanks @TheWind for your comprehensive feedback. I'll read it through tomorrow. Have to get some rest now 

44 minutes ago, TheWind said:

On the contrary, I think that it's important to take a second look and even a third look over your code to improve its performance and possibly find/fix problems you may not have seen the first time. 

Trying to find issues in code that perfectly meets the expected requirements is a waste of time, time that could be spent improving / extending the requirements or on some different project altogether.

Rewriting the same application twenty times because you are obsessed by non-critical parts , optimisation or even style isn't nearly as productive or valuable as writing twenty different applications.

(not saying code review can't be productive, but at this level: write as many scripts as you possibly can and enjoy!)

 

9 minutes ago, Botre said:

Trying to find issues in code that perfectly meets the expected requirements is a waste of time, time that could be spent improving / extending the requirements or on some different project altogether.

Rewriting the same application twenty times because you are obsessed by non-critical parts , optimisation or even style isn't nearly as productive or valuable as writing twenty different applications.

(not saying code review can't be productive, but at this level: write as many scripts as you possibly can and enjoy!)

 

It's not "style", and learning why code should be written in a certain way is very important to learn at an early stage if you're actually interested in creating large scripts that function well, or learning to program in general. If you're just creating scripts for the purpose of osbot and you have no intention of understanding the meaning behind what the lines of code do beyond a very basic level then by all means just make the script function at some level.

I do think that you're completely correct in saying that making many scripts when you're just starting out is very helpful in learning and may help you learn at a faster pace than just trying to improve one, however, taking even a 5-second glance at this script would allow anybody relatively experienced in making scripts see so many problems. @Jake Slang asked specifically for feedback on his sleep and the logic of his script. If he had not asked I would not have given him that feedback.

49 minutes ago, TheWind said:

It's not "style", and learning why code should be written in a certain way is very important to learn at an early stage if you're actually interested in creating large scripts that function well, or learning to program in general. If you're just creating scripts for the purpose of osbot and you have no intention of understanding the meaning behind what the lines of code do beyond a very basic level then by all means just make the script function at some level.

I do think that you're completely correct in saying that making many scripts when you're just starting out is very helpful in learning and may help you learn at a faster pace than just trying to improve one, however, taking even a 5-second glance at this script would allow anybody relatively experienced in making scripts see so many problems. @Jake Slang asked specifically for feedback on his sleep and the logic of his script. If he had not asked I would not have given him that feedback.

Ugh agree to disagree.

Ok, so I even found another bug in here. Once you your inventory is full, it'll execute the bank() method as expected, and when you depositAll(), it'll trigger the canFishShrimps() method, because when you deposit all, your inventory doesn't have that fishing net that it expects.. But since I didn't include an else if statement that conditionally asks if the (getBank().isOpen()) method is true, it would execute the else statement, thus logging out. So in the end refactoring did in fact help.

This is my code after listening to @TheWind's advice:

 

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.api.model.Entity;
import org.osbot.rs07.api.map.Position;
import org.osbot.rs07.script.MethodProvider;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;

import java.util.function.BooleanSupplier;

@ScriptManifest(author = "Jake Slang", name = "JakesShrimpCatcher", info = "Catches shrimp (and anchovies) in Lumbridge swamp.", version = 0.1, logo = "")
public final class JakesShrimpCatcher extends Script  {

    private final Area lumbyFishing = new Area(3240, 3150, 3238, 3145);
    private final Position lumbyBank = new Position(3208, 3220, 2);
    Entity fishingSpot = null;

    @Override
    public final void onStart() {
        log("Is there anybody actually reading THIS?!");
    }

    @Override
    public final int onLoop() throws InterruptedException {
        if (canFishShrimps()) {
            fish();
        } else {
            bank();
        }

        return random(200, 300);
    }

    @Override
    public final void onExit() {
        log("I guess YOU did!");
    }

    private void fish() {
        if (!lumbyFishing.contains(myPosition())) {
            getWalking().webWalk(lumbyFishing);
        }

        fishingSpot = getNpcs().closest("Fishing spot");

        if (fishingSpot != null && fishingSpot.interact("Net")) {
            sleepConditional();
        }

    }

    private void sleepConditional() {
        Sleep.sleepUntil(() -> myPlayer().isAnimating() || !fishingSpot.exists(), 5000);
    }

    private void bank() throws InterruptedException {
        if (getInventory().isFull()) {
            if (!Banks.LUMBRIDGE_UPPER.contains(myPosition())) {
                getWalking().webWalk(lumbyBank);
            }
            if (!getBank().isOpen()) {
                getBank().open();
                getBank().depositAll();
            } else if (getBank().isOpen()) {
                getBank().depositAll();
            } else {
                stop(true);
            }
        }
    }

    private boolean canFishShrimps() throws InterruptedException {
        if (!getInventory().contains("Small fishing net")) {
            if (!Banks.LUMBRIDGE_UPPER.contains(myPosition())) {
                getWalking().webWalk(lumbyBank);
            }
            if (!getBank().isOpen()) {
                getBank().open();
                getBank().withdraw("Small fishing net", 1);
            } else if (getBank().isOpen()) {
                getBank().withdraw("Small fishing net", 1);
            } else {
                stop(true);
            }
        }

        return !myPlayer().isAnimating() && !getInventory().isFull();
    }

}

class Sleep extends ConditionalSleep {

    private final BooleanSupplier condition;

    public Sleep(final BooleanSupplier condition, final int timeout) {
        super(timeout);
        this.condition = condition;
    }

    @Override
    public final boolean condition() throws InterruptedException {
        return condition.getAsBoolean();
    }

    public static boolean sleepUntil(final BooleanSupplier condition, final int timeout) {
        return new Sleep(condition, timeout).sleep();
    }

}

 

Edited July 30, 20178 yr by Jake Slang

There can be a bit more done here to make it a bit more readable for the users (using code plugin ).
As for the script itself, it really isn't such a bad script (if it was your first script and or you're new to programming). Obviously, you learn as you keep on practicing. Depending  on if you wanted to expand the script, I'd suggest you add either classes and create a "Node" or "Task" (they're both practically the same) based system and or an Enum system (in your one and only class) to be able to add more complex "features" as you get better. 

Because everyone has their own style of coding, I can't really comment on the layout too much. Because what I might like, might not be what you like. 

If you're interested in using enums to control your code, you can set it up like so

enum State {
  BANK,
  FISH, 
  IDLE
}

private State getState(){
  if(canFishShrimps()){
    return State.FISH;
  }else{
    return State.BANK;
  }
  
  return State.IDLE;

}


//in your onloop you basically call it like so

switch(getState()){
  case FISH:
	fish();
	break;
  case BANK:
  bank();
  break;
  
  case IDLE:
  break;
}

/*Notice how we have breaks after every case, this is what's called a case system. Whatever the state is, it will equal that certain case
cases are pretty much short for if(getState() == State.FISH) (that's what the case pretty much does, it's just done in a way which makes it more readable)

Anything before that will be called and executed*/

This is not required but depending on the complexity of the script, you might want to consider using one of these methods to make your code look more fancy. You can setup your code to however you want it, but it does tend to get messy and unreadable after a certain amount of lines, making it harder for other people to read.  

Because I'm unsure of how new you are to programming (basing it off this thread only), I gave you the states example because it tends to make sense to newer programmers. I had used this system when I had first started off in programming and it's a very good system to practice with. I then started out using classes and it just went on from there. Bare in mind that if your script is basic and small, there is no need to waste a lot of time setting up your classes. For instance, I make most of my scripts in main only because they're very basic. I had made a few minigame scripts which was more complex and required classes in order to keep it tidy and readable for myself and potentially others. Because if you were coding at 5am and you have 2k lines of code in main, chances are that you would've forgotten what you did after you wake up the following day.

 

Your script doesn't look too bad, I suggest you use more conditional sleeps. For example, you could use sleepUntil() -> getBank().isOpen(), 3000); in conjunction with getBank().open(); because for each loop the script makes, if the bank isn't open by the time it reaches the getbank().open again, it will simple click the bank again. Using a conditional sleep after that, will make it wait a certain amount of time (preventing it from looping again). In this example, 3000ms is equal to 3 seconds, which will wait at this line sleepUntil() -> getBank().isOpen(), 3000);  for 3 seconds (if the bank is not open) and the second the bank opens, it will automatically kill the sleep and continue to the next line of code (in the same loop!) 

and for your script it would be 

  getBank().withdraw("Small fishing net", 1);

Conditional sleeps are very handy and should be used in almost every scenario, as they help your bot prevent spam clicking.

 

 

On 30-7-2017 at 1:02 PM, Satire said:

There can be a bit more done here to make it a bit more readable for the users (using code plugin ).
As for the script itself, it really isn't such a bad script (if it was your first script and or you're new to programming). Obviously, you learn as you keep on practicing. Depending  on if you wanted to expand the script, I'd suggest you add either classes and create a "Node" or "Task" (they're both practically the same) based system and or an Enum system (in your one and only class) to be able to add more complex "features" as you get better. 

Because everyone has their own style of coding, I can't really comment on the layout too much. Because what I might like, might not be what you like. 

If you're interested in using enums to control your code, you can set it up like so

enum State {
  BANK,
  FISH, 
  IDLE
}

private State getState(){
  if(canFishShrimps()){
    return State.FISH;
  }else{
    return State.BANK;
  }
  
  return State.IDLE;

}


//in your onloop you basically call it like so

switch(getState()){
  case FISH:
	fish();
	break;
  case BANK:
  bank();
  break;
  
  case IDLE:
  break;
}

/*Notice how we have breaks after every case, this is what's called a case system. Whatever the state is, it will equal that certain case
cases are pretty much short for if(getState() == State.FISH) (that's what the case pretty much does, it's just done in a way which makes it more readable)

Anything before that will be called and executed*/

This is not required but depending on the complexity of the script, you might want to consider using one of these methods to make your code look more fancy. You can setup your code to however you want it, but it does tend to get messy and unreadable after a certain amount of lines, making it harder for other people to read.  

Because I'm unsure of how new you are to programming (basing it off this thread only), I gave you the states example because it tends to make sense to newer programmers. I had used this system when I had first started off in programming and it's a very good system to practice with. I then started out using classes and it just went on from there. Bare in mind that if your script is basic and small, there is no need to waste a lot of time setting up your classes. For instance, I make most of my scripts in main only because they're very basic. I had made a few minigame scripts which was more complex and required classes in order to keep it tidy and readable for myself and potentially others. Because if you were coding at 5am and you have 2k lines of code in main, chances are that you would've forgotten what you did after you wake up the following day.

 

Your script doesn't look too bad, I suggest you use more conditional sleeps. For example, you could use sleepUntil() -> getBank().isOpen(), 3000); in conjunction with getBank().open(); because for each loop the script makes, if the bank isn't open by the time it reaches the getbank().open again, it will simple click the bank again. Using a conditional sleep after that, will make it wait a certain amount of time (preventing it from looping again). In this example, 3000ms is equal to 3 seconds, which will wait at this line sleepUntil() -> getBank().isOpen(), 3000);  for 3 seconds (if the bank is not open) and the second the bank opens, it will automatically kill the sleep and continue to the next line of code (in the same loop!) 

and for your script it would be 

  getBank().withdraw("Small fishing net", 1);

Conditional sleeps are very handy and should be used in almost every scenario, as they help your bot prevent spam clicking.

 

 

Please no states. 

2 hours ago, Prolax said:

Please no states. 

Did you even read my whole message?  States are good for beginners,  what's the problem here? 

 

Because not everyone understands inheritance.