Code Review (Complete Noob)

[sellout_89]

Hey just wanted to get people opinions on a very basic script. Are there better ways to structure the code, better ways to go about what this script is attempting(Red chins)?

 

import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.model.GroundItem;
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 = "Sellout", name = "Red chin", info = "Basic red chin script", version = 0.1, logo = "")
public final class RedChin extends Script  {
    private long startTime;

    Area top = new Area(2496, 2907, 2496, 2907);
    Area left = new Area(2495, 2906, 2495, 2906);
    Area right = new Area(2497, 2906, 2497, 2906);
    Area bottom = new Area(2496, 2905, 2496, 2905);

    Area[] trapPositions = {top, left, right, bottom};

    @Override
    public final int onLoop() throws InterruptedException {
        resetTraps();
        resetDecayed();
        collectChins();
        layTraps();
        return random(500, 1250);
    }

    @Override
    public final void onStart() {
        log("This will be printed to the logger when the script starts");
        startTime = System.currentTimeMillis();
    }

    @Override
    public void onPaint(final Graphics2D g) {
        Point mP = getMouse().getPosition();
        final long runTime = System.currentTimeMillis() - startTime;

        g.setColor(Color.RED);
        g.drawLine(mP.x - 5, mP.y + 5, mP.x + 5, mP.y - 5);
        g.drawLine(mP.x + 5, mP.y + 5, mP.x - 5, mP.y - 5);

        g.drawString(formatTime(runTime), 5, 336);
    }

    @Override
    public final void onExit() {
        log("This will be printed to the logger when the script exits");
    }

    public final void conditionalSleep(int time){
        new ConditionalSleep(time) {
            @Override
            public boolean condition() {
                return false;
            }
        }.sleep();
    }

    public final String formatTime(final long ms){
        long s = ms / 1000, m = s / 60, h = m / 60;
        s %= 60; m %= 60; h %= 24;
        return String.format("%02d:%02d:%02d", h, m, s);
    }

    public void layTraps(){
        for (Area position:trapPositions) {
            RS2Object trapTwo = getObjects().closest(position, "Shaking box", "Box trap");
            GroundItem trap = getGroundItems().closest(position, "Box trap");
            if(trapTwo == null && trap == null){
                getWalking().webWalk(position);
                conditionalSleep(random(1250, 1500));
                getInventory().interact("Lay", "Box trap");
                conditionalSleep(random(4500, 4750));
            }
        }
    }

    public void resetTraps(){
        for (Area position:trapPositions) {
            RS2Object trap = getObjects().closest(position, "Box trap");
            if(trap != null && trap.getId() == 9385) {
                trap.interact("Reset");
                conditionalSleep(random(8000, 8200));
            }
        }
    }

    public void collectChins(){
        for (Area position:trapPositions) {
            RS2Object trap = getObjects().closest(position,"Shaking box");
            if(trap != null && trap.getId() == 9383){
                trap.interact("Check");
                conditionalSleep(random(2250, 2500));
            }
        }
    }

    public void resetDecayed(){
        for (Area position:trapPositions) {
            GroundItem trap = getGroundItems().closest(position, "Box trap");
            if(trap != null) {
                trap.interact("Lay");
                conditionalSleep(random(7750, 8000));
            }
        }
    }

}

 

Edited August 25, 2018 by sellout_89

[ProjectPact]

You'll find out real fast this won't work. Take it from someone with a lot of experience with Hunter:

 

getObjects().closest

[sellout_89]

11 minutes ago, ProjectPact said:

You'll find out real fast this won't work. Take it from someone with a lot of experience with Hunter:

 

getObjects().closest

Oh what do you exactly mean by that, does the method itself break all the time? What would you recommend instead?

[ProjectPact]

1 hour ago, sellout_89 said:

Oh what do you exactly mean by that, does the method itself break all the time? What would you recommend instead?

How is the bot supposed to know your trap vs. a nearby player's trap?

Edited August 26, 2018 by ProjectPact

[Antonio Kala]

Have you ran it and tested it?

[Ragboys is back]

The first thing I saw: 

 @Override
    public final int onLoop() throws InterruptedException {
        resetTraps();
        resetDecayed();
        collectChins();
        layTraps();
        return random(500, 1250);
    }

You are telling the script what to do, but not WHEN to do it.

Get some conditions in there m8!

[yfoo]

public final void conditionalSleep(int time){
        new ConditionalSleep(time) {
            @Override
            public boolean condition() {
                return false;
            }
        }.sleep();
    }

You don't use conditional sleeps correctly. Any time you call this, all you do is sleep (do nothing) for <time> milliseconds, if you want that functionality use sleep(<time in MS>). Conditional sleeps until its "inner condition function" ( aka abstract condition method) returns true. For example I may want in a red chins script to sleep until the condition that a box trap that I've placed has been triggered by a chin resolves to true.

Furthermore on the matter of whether the box trap is yours, your script needs to register if your character has placed the trap (trap is removed from inventory and placed on the ground). If your character did, store the trap's position in some data structure. When your script runs the part of your code that checks triggered traps, first check whether said trap is yours. Only interact with the trap if the trap's position is stored your data structure, then remove the trap's registration.

 

 

[sellout_89]

Thanks for the feedback so far I'll continue to update the script and post an update.

Edited August 26, 2018 by sellout_89

[ProjectPact]

1 hour ago, PayPalMeRSGP said:

 

Furthermore on the matter of whether the box trap is yours, your script needs to register if your character has placed the trap (trap is removed from inventory and placed on the ground). If your character did, store the trap's position in some data structure. When your script runs the part of your code that checks triggered traps, first check whether said trap is yours. Only interact with the trap if the trap's position is stored your data structure, then remove the trap's registration.

 

 

 

You can't forget about all the cases though. This is what makes it challenging. Actually knowing it is your trap is different than saying if your trap is located on a square you specified. For instance, if you get your spot crashed while you are laying a trap, how will you render which one is your trap and which one is not, because a trap is placed regardless? Or handling moving locations if the spot becomes a nulled tile. Or if the trap vanishes. Or if your trap failed while placing a trap. There are so many of these different scenarios it is comical.