Looking for feedback on first script

[lrdblk]

Hey everyone!

 

Just getting back into java, mostly looking for feedback on this wood cutting script I wrote.  Any productive criticism is welcome.  

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.model.RS2Object;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;

import java.awt.*;

@ScriptManifest(author = "LRDBLK", info = "Chops wood", name = "Wood Chopper", version = 1, logo = "")
public class main extends Script {


    @[member=Override]
    public void onStart() {

        log("Script is starting!");

    }

    @[member=Override]
    public int onLoop() throws InterruptedException {
        Area lumbridgeTrees = new Area(3176, 3238, 3200, 3207);

        if(inventory.isFull()){
            inventory.dropAll();
        }


        if (!lumbridgeTrees.contains(myPlayer())){
            log("walking to area now");
            getWalking().webWalk(lumbridgeTrees.getRandomPosition());

        }else{
            RS2Object tree = getObjects().closest(lumbridgeTrees, "Tree");
            chopTree(tree);

        }

        return random(200, 300);
    }

    @[member=Override]
    public void onExit() {
        log("RawR");
    }

    @[member=Override]
    public void onPaint(Graphics2D g) {

    }

    private Boolean ableToWork(){
        return !myPlayer().isAnimating() && !myPlayer().isMoving();
    }

    private void chopTree(RS2Object tree){

        if(tree != null && ableToWork()){
            log("Picked new tree!");
            tree.interact("Chop down");
        }

        new ConditionalSleep(3000){
            public boolean condition(){
                return myPlayer().isAnimating();
            }
        }.sleep();


    }


}

A few questions I had:

 

If i make a GUI, should it be in a separate class?

 

Am I using ConditionalSleep correctly?

 

Would using States make this script better?

 

If I have a complex script, should I put all my functions in a separate module?

 

 

 

[Juggles]

I like GUI in another class bc its more organized

[Team Cape]

Put the lumbridgeTrees area right below the class definition (public class main...) so it doesn't need to be remade each loop.

 

Also, ableToWork() should return a boolean not a Boolean - there's an important distinction, though Java allows both. Also, try conditioning your conditionalsleep on interaction e.g.

 

if(tree.interact("Chop-down")) {

 new ConditionalSleep(5000).sleep();

}

 

This is pretty good for a first go though gotta say that my first script took me about 2 weeks to perfect and it was a very simple one lol.

 

If i make a GUI, should it be in a separate class?

yes - it's cleaner

 

Am I using ConditionalSleep correctly?

basically

 

Would using States make this script better?

yes

 

If I have a complex script, should I put all my functions in a separate module?

You need to elaborate what you mean by 'function' because if you mean method, then definitely not. If you mean something like banking, then the answer is yes, depending on how large the script is.

[BoysNoize]

Semantics look good to me. Try to keep your code looking clean, here's what it looks like fixed up. Check out the Java code conventions.

 

If i make a GUI, should it be in a separate class?

 

See last answer.

 

Am I using ConditionalSleep correctly?

 

Yup! Make sure that your CSleep has conditionals which check all of the edge cases you can think of. Also, it's nice to make a cleaner CSleep function in another class so you can call something along the lines of sleep(Time, Condition).

 

Would using States make this script better?

 

You are using states in some sense. When you're checking if your inventory is full, you're checking if you're in that state. Sometimes it's nice to make the states explicit for code readability. If you want to expand the script to be more complex, I'd advise using states.

 

If I have a complex script, should I put all my functions in a separate module?

 

Yup, makes it a lot easier to manage everything. 

Edited January 17, 2017 by BoysNoize

[lrdblk]

Semantics look good to me. Try to keep your code looking clean, here's what it looks like fixed up. Check out the Java code conventions.

 

 

See last answer.

 

 

Yup! Make sure that your CSleep has conditionals which check all of the edge cases you can think of. Also, it's nice to make a cleaner CSleep function in another class so you can call something along the lines of sleep(Time, Condition).

 

 

You are using states in some sense. When you're checking if your inventory is full, you're checking if you're in that state. Sometimes it's nice to make the states explicit for code readability. If you want to expand the script to be more complex, I'd advise using states.

 

 

Yup, makes it a lot easier to manage everything. 

 

 

Thanks I appreciate your input! Good call on commenting the shit out of the code.  I'll probably forget everything I did by morning lol

 

Can you give me an example of what your talking about regarding the CSleep?

[BoysNoize]

Thanks I appreciate your input! Good call on commenting the shit out of the code.  I'll probably forget everything I did by morning lol

 

Can you give me an example of what your talking about regarding the CSleep?

 

Commenting is the best thing ever, I see so many people not doing it and I wonder how they get anything done lol.

 

I have a class with a few wee utility functions, one of which is sleep.

public static void sleep(int maxSleep, int deviation, BooleanSupplier condition) {
    new ConditionalSleep(maxSleep, deviation) {
        @[member='Override']
        public boolean condition() throws InterruptedException {
            return condition.getAsBoolean();
        }
    }.sleep();
}

Now I can just call

ClassName.sleep(1000, 200, () -> myPlayer().isAnimating)

for example.

Edited January 17, 2017 by BoysNoize

[lrdblk]

Put the lumbridgeTrees area right below the class definition (public class main...) so it doesn't need to be remade each loop.

 

Also, ableToWork() should return a boolean not a Boolean - there's an important distinction, though Java allows both. Also, try conditioning your conditionalsleep on interaction e.g.

 

if(tree.interact("Chop-down")) {

 new ConditionalSleep(5000).sleep();

}

 

This is pretty good for a first go though gotta say that my first script took me about 2 weeks to perfect and it was a very simple one lol.

 

If i make a GUI, should it be in a separate class?

yes - it's cleaner

 

Am I using ConditionalSleep correctly?

basically

 

Would using States make this script better?

yes

 

If I have a complex script, should I put all my functions in a separate module?

You need to elaborate what you mean by 'function' because if you mean method, then definitely not. If you mean something like banking, then the answer is yes, depending on how large the script is.

 

Thanks for the pointers.  What's the difference between boolean and Boolean?  If Java doesn't care, why should I?

Commenting is the best thing ever, I see so many people not doing it and I wonder how they get anything done lol.

 

I have a class with a few wee utility functions, one of which is sleep.

public static void sleep(int maxSleep, int deviation, BooleanSupplier condition) {
    new ConditionalSleep(maxSleep, deviation) {
        @[member='Override']
        public boolean condition() throws InterruptedException {
            return condition.getAsBoolean();
        }
    }.sleep();
}

Now I can just call

ClassName.sleep(1000, 200, () -> myPlayer().isAnimating)

for example.

 

 

Oh shit that is handy, thanks for the help!  Deviation is basically saying +/- 200 ms right?

Edited January 17, 2017 by lrdblk

[BoysNoize]

Thanks for the pointers.  What's the difference between boolean and Boolean?  If Java doesn't care, why should I?

 

boolean is a primitive type, which means its representation in memory is just a data value. Boolean is not a primitive type, it's a wrapper for boolean. Since it's not primitive, a Boolean is represented in memory as a pointer to a data value, or as many pointers to many different data values. When checking Booleans the value gets unwrapped, that is to say, the Boolean tries to get the primitive value stored at the location of the memory pointer which represents the boolean value attached to the Boolean. Java doesn't care so much, but you should care because Boolean can be null, whereas boolean can not be. 

 

Oh shit that is handy, thanks for the help!  Deviation is basically saying +/- 200 ms right?

 

Yup!

[lrdblk]

boolean is a primitive type, which means its representation in memory is just a data value. Boolean is not a primitive type, it's a wrapper for boolean. Since it's not primitive, a Boolean is represented in memory as a pointer to a data value, or as many pointers to many different data values. When checking Booleans the value gets unwrapped, that is to say, the Boolean tries to get the primitive value stored at the location of the memory pointer which represents the boolean value attached to the Boolean. Java doesn't care so much, but you should care because Boolean can be null, whereas boolean can not be. 

 

 

Yup!

 

 

God damn nulls!

[Team Cape]

Thanks for the pointers.  What's the difference between boolean and Boolean?  If Java doesn't care, why should I?

 

Boolean is an object (hence the uppercase first character), and boolean is a primitive (lowercase).

 

'Java' does care; they have allowed programmers to (mostly) interchange the two when necessary, however, without much trouble. Where it's not necessary, it's pointless, and that's exactly what you're doing. 

 

Where it would be useful:

List<Boolean> bools = new ArrayList<>();

bools.add(true);

 

Where it's unnecessary:

Boolean[] bools = new Boolean[5]; //insetad of boolean[] bools = new boolean[5];

[lrdblk]

Boolean is an object (hence the uppercase first character), and boolean is a primitive (lowercase).

 

'Java' does care; they have allowed programmers to (mostly) interchange the two when necessary, however, without much trouble. Where it's not necessary, it's pointless, and that's exactly what you're doing. 

 

Where it would be useful:

List<Boolean> bools = new ArrayList<>();

bools.add(true);

 

Where it's unnecessary:

Boolean[] bools = new Boolean[5]; //insetad of boolean[] bools = new boolean[5];

 

 

Interesting, thanks for the right up!

[Explv]

Hey everyone!

 

Just getting back into java, mostly looking for feedback on this wood cutting script I wrote.  Any productive criticism is welcome.  

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.model.RS2Object;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;

import java.awt.*;

@ScriptManifest(author = "LRDBLK", info = "Chops wood", name = "Wood Chopper", version = 1, logo = "")
public class main extends Script {


    @[member='Override']
    public void onStart() {

        log("Script is starting!");

    }

    @[member='Override']
    public int onLoop() throws InterruptedException {
        Area lumbridgeTrees = new Area(3176, 3238, 3200, 3207);

        if(inventory.isFull()){
            inventory.dropAll();
        }


        if (!lumbridgeTrees.contains(myPlayer())){
            log("walking to area now");
            getWalking().webWalk(lumbridgeTrees.getRandomPosition());

        }else{
            RS2Object tree = getObjects().closest(lumbridgeTrees, "Tree");
            chopTree(tree);

        }

        return random(200, 300);
    }

    @[member='Override']
    public void onExit() {
        log("RawR");
    }

    @[member='Override']
    public void onPaint(Graphics2D g) {

    }

    private Boolean ableToWork(){
        return !myPlayer().isAnimating() && !myPlayer().isMoving();
    }

    private void chopTree(RS2Object tree){

        if(tree != null && ableToWork()){
            log("Picked new tree!");
            tree.interact("Chop down");
        }

        new ConditionalSleep(3000){
            public boolean condition(){
                return myPlayer().isAnimating();
            }
        }.sleep();


    }


}

A few questions I had:

 

If i make a GUI, should it be in a separate class?

 

Am I using ConditionalSleep correctly?

 

Would using States make this script better?

 

If I have a complex script, should I put all my functions in a separate module?

 

Firstly the answers to your questions:

 

If i make a GUI, should it be in a separate class?

 

It's up to you, it would be cleaner if it was in a separate class.

 

Am I using ConditionalSleep correctly?

 

Yes and no, the ConditionalSleep itself is fine, but you are calling it in the wrong place. You should only call the ConditionalSleep after you have checked the tree is not null and also after you have checked the interaction with the tree was successful. Otherwise your script will sleep when it shouldn't.

 

Would using States make this script better?

 

No, personally I don't think anyone should use States, because you are separating the condition from the action, which just makes it harder to read. I think that when your script becomes more complex, you should separate it out into classes and methods accordingly. 

 

If I have a complex script, should I put all my functions in a separate module?

 

I don't understand what you mean by this question, consider reading some basic OOP tutorials.

 

 

Other issues I can see looking at your code:

 

1. Remove the onStart() and onEnd() methods considering you aren't doing anything useful in them
2. Declare the Area lumbridgeTrees as a private global variable so that you aren't redefining it every time onLoop is called
3. After you do the check for if the inventory is full, I would probably make the next if statement an else if
4. You don't need to web walk to a random position in the Area lumbridgeTrees, just call webWalk(lumbridgeTrees)
5. You probably want to check if the player should chop a new tree (your ableToWork()) method before finding a new Tree, otherwise you are just constantly looking for the closest tree when you don't even want to chop it.
6. You should only perform the conditional sleep after the .interact("Chop down") method has returned true. 
7. You should return boolean instead of Boolean from the method ableToWork()
8. You should follow Java naming conventions, (the name of your class should start with an uppercase letter). Also, although it doesn't particularly matter in this context, I would give your Script class a better name than "Main", perhaps "Woodchopper" would be more appropriate.

[lrdblk]

 

Firstly the answers to your questions:

 

If i make a GUI, should it be in a separate class?

 

It's up to you, it would be cleaner if it was in a separate class.

 

Am I using ConditionalSleep correctly?

 

Yes and no, the ConditionalSleep itself is fine, but you are calling it in the wrong place. You should only call the ConditionalSleep after you have checked the tree is not null and also after you have checked the interaction with the tree was successful. Otherwise your script will sleep when it shouldn't.

 

Would using States make this script better?

 

No, personally I don't think anyone should use States, because you are separating the condition from the action, which just makes it harder to read. I think that when your script becomes more complex, you should separate it out into classes and methods accordingly. 

 

If I have a complex script, should I put all my functions in a separate module?

 

I don't understand what you mean by this question, consider reading some basic OOP tutorials.

 

 

Other issues I can see looking at your code:

 

1. Remove the onStart() and onEnd() methods considering you aren't doing anything useful in them
2. Declare the Area lumbridgeTrees as a private global variable so that you aren't redefining it every time onLoop is called
3. After you do the check for if the inventory is full, I would probably make the next if statement an else if
4. You don't need to web walk to a random position in the Area lumbridgeTrees, just call webWalk(lumbridgeTrees)
5. You probably want to check if the player should chop a new tree (your ableToWork()) method before finding a new Tree, otherwise you are just constantly looking for the closest tree when you don't even want to chop it.
6. You should only perform the conditional sleep after the .interact("Chop down") method has returned true. 
7. You should return boolean instead of Boolean from the method ableToWork()
8. You should follow Java naming conventions, (the name of your class should start with an uppercase letter). Also, although it doesn't particularly matter in this context, I would give your Script class a better name than "Main", perhaps "Woodchopper" would be more appropriate.

 

 

 

Thanks for your input, super useful!  I'm actually in the process of doing a lot of what you said

 

I understand your point on States but decided to use them as an intermediary step before learning how to separate in different classes

 

Can anyone tell me why this is throwing any error?

private void depositBank(){

        log("banking!");
        if(!getBank().isOpen()){
            getBank().open();
            sleep(3000, 500, () -> getBank().isOpen());
        }else{
            getBank().depositAll();

        }

    }

Error:(134, 27) java: unreported exception java.lang.InterruptedException; must be caught or declared to be thrown on getBank().open

[Explv]

Thanks for your input, super useful!  I'm actually in the process of doing a lot of what you said

 

I understand your point on States but decided to use them as an intermediary step before learning how to separate in different classes

 

Can anyone tell me why this is throwing any error?

private void depositBank(){

        log("banking!");
        if(!getBank().isOpen()){
            getBank().open();
            sleep(3000, 500, () -> getBank().isOpen());
        }else{
            getBank().depositAll();

        }

    }

Error:(134, 27) java: unreported exception java.lang.InterruptedException; must be caught or declared to be thrown on getBank().open

getBank().open()

Throws an InterruptedException. You either need to catch the exception or make your method throw an InterruptedException as well

[lrdblk]

getBank().open()

Throws an InterruptedException. You either need to catch the exception or make your method throw an InterruptedException as well

 

 

 

Which is considered best practise ?