Feedback for my first script

[ultraswagger]

Hi everyone,

I'm just getting started writing scripts for RS and wrote my first script to auto fish and deposit in bank. Would love to get some feedback on anything I could be doing better.

import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.api.model.NPC;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;
import org.osbot.rs07.utility.ConditionalSleep;
import java.util.Comparator;
import java.util.Optional;
import java.util.Random;

@ScriptManifest(name = "FirstScript", author = "ultraswagger", version = 1.0, info = "", logo = "")
public class FirstScript extends Script {
    private static Random gaussianGenerator = new Random();

    private Comparator closestCageFishingSpot = (Comparator<NPC>) (a, b) ->
            getMap().distance(a.getPosition()) - getMap().distance(b.getPosition());

    private ConditionalSleep bankConditionalSleep = new ConditionalSleep(5000) {
        @Override
        public boolean condition() throws InterruptedException {
            return getBank().isOpen();
        }
    };

    private ConditionalSleep fishingConditionalSleep = new ConditionalSleep(5000) {
        @Override
        public boolean condition() throws InterruptedException {
            return getInventory().isFull();
        }
    };

    private void depositFishToBank() {
        if(!Banks.CATHERBY.contains(myPosition()))
            getWalking().webWalk(Banks.CATHERBY);
        else {
            if(!getBank().isOpen())
                bankConditionalSleep.sleep();
            else
                getBank().depositAllExcept("Lobster pot");
        }
    }

    @Override
    public void onStart() {
        log("Starting Script");
    }

    @Override
    public int onLoop() {
        if (!this.myPlayer().isAnimating()) {
            if (this.getInventory().isFull()) {
                depositFishToBank();
            } else {
                Optional<NPC> cageFishingSpot = this.getNpcs().getAll().stream()
                        .filter(o -> o.hasAction("Cage")).min(closestCageFishingSpot);

                if (cageFishingSpot.isPresent()) {
                    cageFishingSpot.get().interact("Cage");
                    fishingConditionalSleep.sleep();
                }
            }
        }

        return (int)((double)1000 * gaussianGenerator.nextGaussian());
    }

    @Override
    public void onExit() {
        log("Script has been stopped");
    }
}

Thanks in advance!

Edited February 2, 2020 by ultraswagger

[BravoTaco]

How is your artifacts tab set up?

The .class file is the wrong thing to be placing into the Scripts folder. You need to build out a .jar and have that in their.

Example of the artifacts tab:

Edited February 2, 2020 by BravoTaco

[ultraswagger]

1 hour ago, BravoTaco said:

How is your artifacts tab set up?

The .class file is the wrong thing to be placing into the Scripts folder. You need to build out a .jar and have that in their.

Example of the artifacts tab:

Thanks! That fixed it for me . Not sure why but the tutorials I was looking at said to copy the .class file over. 

How's my code though? Anything I could do better?

[BravoTaco]

Only thing I noticed is that you use 'this' keyword when its not needed. Usually 'this' is used within methods that have parameter names that are the same as declared variable names in the class.

Example:

public class Float3 {
    private float x, y, z;

    public Float3(float x, float y, float z) {
        this.x = x;
        this.y = y;
        this.z = z;
    }

    public float getX() {
        return x;
    }

    public float getY() {
        return y;
    }

    public float getZ() {
        return z;
    }

    public Float3 add(Float3 other) {
        x += other.x;
        y += other.y;
        z += other.z;
        return this;
    }

    public Float3 addNumberEqually(float numberToEquallyAdd) {
        Float3 temp = new Float3(numberToEquallyAdd, numberToEquallyAdd, numberToEquallyAdd);
        //this.add(temp); 'this' is not needed here.
        add(temp);
        return this;
    }
    
}

Edited February 2, 2020 by BravoTaco

[Camaro]

Optional<NPC> cageFishingSpot = this.getNpcs().getAll().stream()
		.filter(o -> o.hasAction("Cage")).min(closestCageFishingSpot);

if (cageFishingSpot.isPresent()) {
	cageFishingSpot.get().interact("Cage");
	fishingConditionalSleep.sleep();
}

can be replaced with

NPC cageFishingSpot = getNpcs().closest(o -> o != null && o.hasAction("Cage"));

if (cageFishingSpot != null && cageFishingSpot.interact("Cage")) {
	fishingConditionalSleep.sleep();
}

Notice How there is a null check in the lambda expression itself. Also, I moved the interact() method into the if condition. Only sleep if the interact() was successful.

[ultraswagger]

3 hours ago, camaro 09 said:

Optional<NPC> cageFishingSpot = this.getNpcs().getAll().stream()
		.filter(o -> o.hasAction("Cage")).min(closestCageFishingSpot);

if (cageFishingSpot.isPresent()) {
	cageFishingSpot.get().interact("Cage");
	fishingConditionalSleep.sleep();
}

can be replaced with

NPC cageFishingSpot = getNpcs().closest(o -> o != null && o.hasAction("Cage"));

if (cageFishingSpot != null && cageFishingSpot.interact("Cage")) {
	fishingConditionalSleep.sleep();
}

Notice How there is a null check in the lambda expression itself. Also, I moved the interact() method into the if condition. Only sleep if the interact() was successful.

Yea that's a better approach. Thanks!