Where can I improve?

[Fugz]

Looking to get back into scripting, here is the last script I made. I was just wondering if I could get some constructive criticism.

 

http://pastebin.com/ixpqLkfa

 

import org.osbot.rs07.api.ui.Skill;
import org.osbot.rs07.api.ui.Spells.NormalSpells;
import org.osbot.rs07.script.Script;
import org.osbot.rs07.script.ScriptManifest;

import java.awt.*;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;
import java.util.concurrent.TimeUnit;

import javax.swing.JButton;
import javax.swing.JComboBox;
import javax.swing.JFrame;
 
@ScriptManifest(author = "Fugz", info = "Teleports to all Standard Magic locations", name = "fTeleporting", version = 0.2, logo = "")
public class main extends Script {
 
	NormalSpells teleType;
	String spell;
	int startEXP;
	int currentEXP;
	int timesTele;
	private int currentLevel;
    private int beginningLevel;
    private int levelsGained;
    private long timeBegan;
    private long timeRan;
	
	private enum State {
		TELE, IDLE
	};
    @Override
    public void onStart() {
    	EventQueue.invokeLater(this::initSettings);

        synchronized(main.class) {
            try {
                main.class.wait(); //forces this thread to wait
            } catch(InterruptedException e) {
                e.printStackTrace();
            }
            log("Welcome to fTeleporting");
            this.experienceTracker.start(Skill.MAGIC);
            main.this.mouse.setSpeed(4);
            this.startEXP = this.skills.getExperience(Skill.MAGIC);
            beginningLevel = skills.getStatic(Skill.MAGIC);
            timeBegan = System.currentTimeMillis();
            
        }
    }
    private void initSettings() {
        JFrame frame = new JFrame("Settings");
        frame.setLayout(new FlowLayout());
        frame.addWindowListener(new WindowAdapter() {
            public void windowClosing(WindowEvent event) {
                frame.dispose();
                main.this.stop();
            }
        });
        frame.setLocationByPlatform(true);

        NormalSpells[] teleLocation = { NormalSpells.VARROCK_TELEPORT, NormalSpells.LUMBRIDGE_TELEPORT, NormalSpells.FALADOR_TELEPORT, NormalSpells.HOUSE_TELEPORT, NormalSpells.CAMELOT_TELEPORT, NormalSpells.ARDOUGNE_TELEPORT, NormalSpells.WATCHTOWER_TELEPORT, NormalSpells.TROLLHEIM_TELEPORT, NormalSpells.TELEPORT_TO_APE_ATOLL, NormalSpells.TELEPORT_TO_KOUREND, NormalSpells.TELEPORT_TO_BOUNTY_TARGET };
        JComboBox<NormalSpells> teleOptionBox = new JComboBox<>(teleLocation);
        frame.add(teleOptionBox);

        JButton button = new JButton("Start");
        button.addActionListener(event -> {
            teleType = (NormalSpells) teleOptionBox.getSelectedItem();
            frame.dispose();
            synchronized(main.class) {
                main.class.notify();
            }
        });
        frame.add(button);
        frame.pack();
        frame.setVisible(true);
    }
    
    private State getState() {
        if (this.inventory.contains("Law rune") && (!myPlayer().isAnimating()))
            return State.TELE;
        else
        return State.IDLE;
    }
    
    @Override
    public int onLoop() throws InterruptedException {
    	switch (getState()) {
    	case TELE:
    	{
    		teleport();
    	}
    	break;
    	case IDLE:
    	{
    idle();
    	}
    	}
        return random(200, 300);
    }
 
    @Override
    public void onExit() {
        log("Thanks for running fTeleporting!");
    }
 
    @Override
    public void onPaint(Graphics2D g) {
    	currentLevel = skills.getStatic(Skill.MAGIC);
        levelsGained = currentLevel - beginningLevel;
        timeRan = System.currentTimeMillis() - this.timeBegan; 
    	g.setColor(new Color(225, 0, 0, 75));
	    g.fill3DRect(271, 237, 245, 100, true);
	    g.setColor(new Color(0, 0, 0));
	    g.drawString("fTeleporting by Fugz", 281, 252);
	    g.drawString("Time running: " + (ft(timeRan)), 281, 267);
	    g.drawString("Time teleported: " + this.timesTele, 281, 282);
	    g.drawString("Task: " + teleType.toString(), 281, 297);
	    g.drawString("EXP Gained: " + this.experienceTracker.getGainedXP(Skill.MAGIC) + " (" + this.experienceTracker.getGainedXPPerHour(Skill.MAGIC) + "/hr)" , 281, 312);
	    g.drawString("Levels Gained: " + beginningLevel + " / " + currentLevel + " ( +" + levelsGained + " )", 281, 327);
    }
    
    private String ft(long duration) 
	{
		String res = "";
		long days = TimeUnit.MILLISECONDS.toDays(duration);
		long hours = TimeUnit.MILLISECONDS.toHours(duration)
		- TimeUnit.DAYS.toHours(TimeUnit.MILLISECONDS.toDays(duration));
		long minutes = TimeUnit.MILLISECONDS.toMinutes(duration)
		- TimeUnit.HOURS.toMinutes(TimeUnit.MILLISECONDS
		.toHours(duration));
		long seconds = TimeUnit.MILLISECONDS.toSeconds(duration)
		- TimeUnit.MINUTES.toSeconds(TimeUnit.MILLISECONDS
		.toMinutes(duration));
		if (days == 0) {
		res = (hours + ":" + minutes + ":" + seconds);
		} else {
		res = (days + ":" + hours + ":" + minutes + ":" + seconds);
		}
		return res;
	} 
    private void teleport() throws InterruptedException{
    	runeCheck();
    	this.magic.open();
    	magic.deselectSpell();
		if (!myPlayer().isAnimating()); {
    		sleep(random(800,1200));
    		this.magic.castSpell(teleType);
    		this.timesTele += 1L;
		}
		currentEXP = (this.skills.getExperience(Skill.MAGIC) - this.startEXP);
		sleep(random(800,1200));
}
    		private void idle() throws InterruptedException{
    			runeCheck();
    		}
    		
	private void runeCheck() throws InterruptedException{
		if (teleType.toString() == "VARROCK_TELEPORT"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Fire rune")){
				stop();
			}else{
				
			}
		}else if(teleType.toString() == "LUMBRIDGE_TELEPORT"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Earth rune")){
				stop();
			}else{
				
			}
		}else if(teleType.toString() == "FALADOR_TELEPORT"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Water rune")){
				stop();
			}else{
			
			}
		}else if(teleType.toString() == "HOUSE_TELEPORT"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Earth rune")){
				stop();
			}else{
				
			}
		}else if (teleType.toString() == "CAMELOT_TELEPORT"){
			if (!this.inventory.contains("Law rune")){
				stop();
			}else{
				
			}
		}else if (teleType.toString() == "ARDOUGNE_TELEPORT"){
			if (!this.inventory.contains("Law rune")){
				stop();
			}else{	
			}
		}else if (teleType.toString() == "WATCHTOWER_TELEPORT"){
			if (!this.inventory.contains("Law rune")){
			stop();	
			}else{
				
			}
		}else if (teleType.toString() == "TROLLHEIM_TELEPORT"){
			if (!this.inventory.contains("Law rune")){
				stop();
			}else{
				
			}
		}else if (teleType.toString() == "TELEPORT_TO_APE_TOLL"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Banana")){
				stop();
			}else{
				
			}
		}else if (teleType.toString() == "TELEPORT_TO_KOUREND"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Soul rune")){
				stop();
			}else{
				
			}
		}else if (teleType.toString() == "TELEPORT_TO_BOUNTY_TARGET"){
			if (!this.inventory.contains("Law rune") || !this.inventory.contains("Death rune") || !this.inventory.contains("Chaos rune")){
				stop();
			}else{
				
			}
		}
	}
}
    	
    	
    
 

 

All help/opinions are appreciated

[KEVzilla]

Formatting. Use a separate class for your GUI and for the rune check use switch (google it).

[Explv]

1) For your runeCheck method you should use a switch not if else statements.

2) When comparing Strings, like in your runeCheck method you should use .equals() not == 

    For Strings, == compares references, where as .equals() compares the values.

3) Your runeCheck method can be replaced with, 

getMagic().canCast(teleport);

4) Formatting.

5) Stop using this. where you don't need to. This. is a reference to the current object.

    It is not needed to refer to fields in the current object, unless you have a parameter with the same name.

6) Use my time formatting method, it looks nicer

public String formatTime(long ms){

    long s = ms / 1000, m = s / 60, h = m / 60, d = h / 24;
    s %= 60; m %= 60; h %= 24;

    return d > 0 ? String.format("%02d:%02d:%02d:%02d", d, h, m, s) :
           h > 0 ? String.format("%02d:%02d:%02d", h, m, s) :
           String.format("%02d:%02d", m, s);
}

7) You don't need to store currentXp, or timeRan, or any variables like that. This information can be calculated when needed, or obtained using the ExperienceTracker and are not used in any other methods. For example there is the gainedLevels and gainedXp methods in ExperienceTracker.

 

8) Seriously, formatting.

 

Edited January 30, 2016 by Explv