"Finished" my second script. Was hoping for feedback

[Cosine]

I've just finished writing my second script which kills cows in Falador farm. It supports healing with Salmon, banking when out of food, and opens the gate when its closed to get into the field. It was intended to train low level pures, which is why it needs Salmon in the bank as you will need to heal eventually.

I was hoping to get some feedback on the code I've written to see if there's anything obvious that I'm doing poorly, or where I could optimize (not a Java programmer or experienced scripter).

package core;

import org.osbot.rs07.api.filter.Filter;
import org.osbot.rs07.api.map.Area;
import org.osbot.rs07.api.map.constants.Banks;
import org.osbot.rs07.api.model.NPC;
import org.osbot.rs07.api.model.RS2Object;
import org.osbot.rs07.api.ui.Skill;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

import java.awt.*;

@ScriptManifest(name = "CowKiller", author = "Cosine", version = 1.0, info = "", logo = "") 

public class Main extends Script {
	
	private long startTime;
	private long runTime;
	
	private Area cowField = new Area(3043, 3313, 3021, 3298);
	private Area gateArea = new Area(3044, 3314, 3025, 3325);
	private int foodId = 329;

    @Override
    public void onStart() {
        startTime = System.currentTimeMillis();
        getExperienceTracker().startAll();
    }

    @Override
    public void onExit() {
    }

    @Override
    public int onLoop() throws InterruptedException {
    	if (atCowField()) {
    		if (lowHealth() && !eatFood()) {
    			if (gateClosed()) {
    				openGate();
    			}
    			walkToBank();
    		} else if (inCombat()) {
    			waitToAttack().sleep();
    		} else if (attackCow()) {
    			waitToAttack().sleep();
    		}
    		
    	} else if (atBank()) {
    		if (canHeal()) {
    			eatFood();
    		} else if (spaceForFood()) {
    			if (!withdrawFood()) {
    				stop(false); //no food left, idle till logout
    			}
    		} else {
    			walkToGate();
    		}
    		
	    } else if (spaceForFood()) {
			walkToBank();
		} else if (atGate()) {
			if (gateExists()) {
				if (gateClosed()) {
					openGate();
				} else {
					walkToCows();
				}
			}
			
		} else {
			walkToGate();
		}
    	
		return(random(300, 400));
    }
    
    //----- [START] AREA/WALKING METHODS [START] -----
    private boolean atBank() {
    	return Banks.FALADOR_EAST.contains(myPlayer());
    }
    
    private boolean atCowField() {
    	return cowField.contains(myPlayer());
    }
    
    private boolean atGate() {
    	return gateArea.contains(myPlayer());
    }
    
    private void walkToBank() {
    	getWalking().walk(Banks.FALADOR_EAST);
    }
    
    private void walkToCows() {
    	getWalking().walk(cowField);
    }
    
    private void walkToGate() {
    	getWalking().walk(gateArea);
    }
    
    private RS2Object fieldGate() {
    	return getObjects().closest("Gate");
    }
    
    private boolean gateClosed() {
    	return fieldGate().hasAction("Open");
    }
    
    private boolean gateExists() {
    	return fieldGate() != null;
    }
    
    private void openGate() {
		if (!fieldGate().isVisible()) {
			camera.toEntity(fieldGate());
		}
		
		fieldGate().interact("Open");
		waitToAttack().sleep();
    }
    //----- [END] AREA/WALKING METHODS [END]  -----
    
    //----- [START] BANKING/EATING METHODS [START] -----
    private boolean foodInBank() {
    	return getBank().contains(foodId);
    }
    
    private boolean withdrawFood() throws InterruptedException {
    	if (getBank().open()) {
    		if (foodInBank()) {
    			getBank().withdraw(foodId, 28);
    			return true;
    		}
    	}
    	return false;
    }
    
    private boolean spaceForFood() {
    	return getInventory().getEmptySlotCount() > 0;
    }
    
    private boolean fullHP() {
    	return getSkills().getDynamic(Skill.HITPOINTS) == getSkills().getStatic(Skill.HITPOINTS);
    }
    
    private boolean lowHealth() {
    	return myPlayer().getHealthPercent() < random(30, 50);
    }
    
    private boolean haveFood() {
    	return getInventory().contains(foodId);
    }
    
    private boolean canHeal() {
    	return !fullHP() && haveFood();
    }
    
    private boolean eatFood() {
    	if (haveFood()) {
    		getInventory().getItem(foodId).interact("Eat");
    		return true;
    	}
    	return false;
    }
    //----- [END] BANKING/EATING METHODS [END] -----
    
    //----- [START] COMBAT METHODS [START] -----
    private boolean inCombat() {
    	return getCombat().isFighting() || myPlayer().isMoving();
    }
    
    private boolean attackCow() {
    	NPC cow = getNpcs().closest(cowFilter());
    	if (cow != null) {
    		if (!cow.isVisible()) {
    			camera.toEntity(cow);
    		}
    		cow.interact("Attack");
    		return true;
    	}
    	return false;
    }
    
    private Filter cowFilter() {
    	return new Filter<NPC>() {
    		@Override
    		public boolean match(NPC npc) {
    			return npc.getName().startsWith("Cow") && npc.isAttackable() && cowField.contains(npc);
    		}
    	};
    }
    
    private Sleep waitToAttack() {
    	return new Sleep(() -> !inCombat(), 5000);
    }
    //----- [END] COMBAT METHODS [END] -----
    
    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);
    }
    
    @Override
    public void onPaint(Graphics2D g) {
    	runTime = System.currentTimeMillis() - startTime;
    	
    	g.setColor(new Color(252, 200, 20, 255));
    	g.fillRect(10, 220, 180, 110);
    	
    	g.setColor(Color.decode("#FFFFFF"));
        g.drawString("Runtime: " + formatTime(runTime), 17, 240);
        g.drawString("Attack XP: " + getExperienceTracker().getGainedXP(Skill.ATTACK) + " (" + getExperienceTracker().getGainedLevels(Skill.ATTACK)+ ")", 17, 260);
        g.drawString("Strength XP: " + getExperienceTracker().getGainedXP(Skill.STRENGTH) + " (" + getExperienceTracker().getGainedLevels(Skill.STRENGTH)+ ")", 17, 280);
        g.drawString("Defence XP: " + getExperienceTracker().getGainedXP(Skill.DEFENCE) + " (" + getExperienceTracker().getGainedLevels(Skill.DEFENCE)+ ")", 17, 300);
        g.drawString("Hitpoints XP: " + getExperienceTracker().getGainedXP(Skill.HITPOINTS) + " (" + getExperienceTracker().getGainedLevels(Skill.HITPOINTS)+ ")", 17, 320);
    }
    
}

Sleep class taken from Explv's Scripting 101

package core;

import org.osbot.rs07.utility.ConditionalSleep;

import java.util.function.BooleanSupplier;

public final 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();
    }
}

I think more specifically I was hoping to hear some ideas about how I could maybe split this up into more classes, as there's so many methods in this class and its a nightmare to scroll through them all. So if you've scripted before, how do you usually split up your bot into classes?

If anyone would like to test that would be great too. Start in Falador bank with a bunch of Salmon in the bank and then it should run cleanly from there!

[dogetrix]

You can create the cowFilter's filter once and cache it so that you don't need to create a new object every time you call the cowFilter method. Saves a tiny amount of resources (negligible maybe), but it's a much better practice.

For these auxiliary methods, like attackCow, instead of returning true, return the last method you attempted, for example return cow.interact("Attack"); instead of your current return true. 

In addition, calling interact turns the camera for you, so you can get rid of those "not isVisible then turn camera" checks.

Edited July 10, 2018 by dogetrix
format

[Rays]

You should definitely look into Conditional Sleeps some more.

Also this:

private boolean atBank() {
    	return Banks.FALADOR_EAST.contains(myPlayer());
    }
    
    private boolean atCowField() {
    	return cowField.contains(myPlayer());
    }
    
    private boolean atGate() {
    	return gateArea.contains(myPlayer());
    }
    
    private void walkToBank() {
    	getWalking().walk(Banks.FALADOR_EAST);
    }
    
    private void walkToCows() {
    	getWalking().walk(cowField);
    }
    
    private void walkToGate() {
    	getWalking().walk(gateArea);
    }
    
    private RS2Object fieldGate() {
    	return getObjects().closest("Gate");
    }
    
    private boolean gateClosed() {
    	return fieldGate().hasAction("Open");
    }
    
    private boolean gateExists() {
    	return fieldGate() != null;
    }

These functions are essentially waste of space since all they do is return a value of another function.

Edited July 10, 2018 by Rays

[Cosine]

7 minutes ago, dogetrix said:

You can create the cowFilter's filter once and cache it so that you don't need to create a new object every time you call the cowFilter method. Saves a tiny amount of resources (negligible maybe), but it's a much better practice. For these auxiliary methods, like attackCow, instead of returning true, return the last method you attempted, for example return cow.interact("Attack"); instead of your current return true. 

On another note, calling interact turns the camera for you, so you can get rid of those "not isVisible then turn camera" checks.

Great idea. Hadn't thought about that. I'll remove the camera checks too, thanks

[Cosine]

8 minutes ago, Rays said:

You should definitely look into Conditional Sleeps some more.

Also this:

private boolean atBank() {
    	return Banks.FALADOR_EAST.contains(myPlayer());
    }
    
    private boolean atCowField() {
    	return cowField.contains(myPlayer());
    }
    
    private boolean atGate() {
    	return gateArea.contains(myPlayer());
    }
    
    private void walkToBank() {
    	getWalking().walk(Banks.FALADOR_EAST);
    }
    
    private void walkToCows() {
    	getWalking().walk(cowField);
    }
    
    private void walkToGate() {
    	getWalking().walk(gateArea);
    }
    
    private RS2Object fieldGate() {
    	return getObjects().closest("Gate");
    }
    
    private boolean gateClosed() {
    	return fieldGate().hasAction("Open");
    }
    
    private boolean gateExists() {
    	return fieldGate() != null;
    }

These functions are essentially waste of space since all they do is return a value of another function.

Is there anything about my ConditionalSleep that you think is bad at all? 

I added those single line methods to try and increase the readability of my onLoop method. I could probably remove them without having big impact on the script readability, but I somewhat like being able to read a method name in a conditional and know instantly what it does/why it's needed

[Rays]

1 hour ago, Cosine said:

Is there anything about my ConditionalSleep that you think is bad at all? 

I added those single line methods to try and increase the readability of my onLoop method. I could probably remove them without having big impact on the script readability, but I somewhat like being able to read a method name in a conditional and know instantly what it does/why it's needed

Personally for me, the methods provided with the API already does a good job on this. It's more of a hassle when other people are reading your script though.

Didn't notice your use of the sleep - my bad. Look good.