Can someone critique my code?

[Heist]

I made a script that trains magic on the wizard tower lesser demon. I'm sure the code is pretty rough to look at and there's probably a lot of practices I need to start doing, but I just don't know what I need to do to become better right now. I was hoping someone could look through my code and give me tips and point some things out that I should/shouldn't do.

Thanks!

Link to the pastebin

[Hawk]

You can significantly shorten some of the methods used while allowing for any other spells you wish to add in the future much more streamlined.

For instance,

	private boolean canAttack() throws InterruptedException {
		if(spell.toString().toLowerCase().equals("firestrike")) {
			return canUseFireStrike();
		}
		if(spell.toString().toLowerCase().equals("earthstrike")) {
			return canUseEarthStrike();
		}
		if(spell.toString().toLowerCase().equals("waterstrike")) {
			return canUseWaterStrike();
		}
		if(spell.toString().toLowerCase().equals("airstrike")) {
			return canUseAirStrike();
		}
		return false;
	}

Can be shortened if you initialize spell to a MagicSpell:

    private boolean canAttack() throws InterruptedException {
        return getMagic().canCast(spell, false);
    }

This

	private void selectSpell() throws InterruptedException {
		if (spell.toString().toLowerCase().equals("firestrike")) {
			RS2Widget w = widgets.get(201, 1, 4);
			if (w != null && w.isVisible()) {
				w.interact("Fire Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("earthstrike")) {
			RS2Widget w = widgets.get(201, 1, 3);
			if (w != null && w.isVisible()) {
				w.interact("Earth Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("waterstrike")) {
			RS2Widget w = widgets.get(201, 1, 2);
			if (w != null && w.isVisible()) {
				w.interact("Water Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("airstrike")) {
			RS2Widget w = widgets.get(201, 1, 1);
			if (w != null && w.isVisible()) {
				w.interact("Wind Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
	}

Can be shortened to

    private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, spell.getSpriteIndex());
        if (spellWidget != null && spellWidget.isVisible()) {
            spellWidget.interact();
        }
        Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
    }

I would move some of the logic implementation in the loop to its own method for readability, so something like this:

    @Override
    public int onLoop() throws InterruptedException {
        dismissDialogue();
        if (!canAttack()) {
            this.stop();
            log("[Error] Out of runes.");
        }
        if (shouldAttack()) {
            if (shouldRotateScreen()) {
                rotateCameraToLesserDemon();
            } else {
                attack();
            }
        }
        return (random(600, 900)); // time before loop starts over
    }

 

[Nbacon]

 ExperienceTracker class also look into that.

[Heist]

6 hours ago, Hawk said:

You can significantly shorten some of the methods used while allowing for any other spells you wish to add in the future much more streamlined.

For instance,

	private boolean canAttack() throws InterruptedException {
		if(spell.toString().toLowerCase().equals("firestrike")) {
			return canUseFireStrike();
		}
		if(spell.toString().toLowerCase().equals("earthstrike")) {
			return canUseEarthStrike();
		}
		if(spell.toString().toLowerCase().equals("waterstrike")) {
			return canUseWaterStrike();
		}
		if(spell.toString().toLowerCase().equals("airstrike")) {
			return canUseAirStrike();
		}
		return false;
	}

Can be shortened if you initialize spell to a MagicSpell:

    private boolean canAttack() throws InterruptedException {
        return getMagic().canCast(spell, false);
    }

This

	private void selectSpell() throws InterruptedException {
		if (spell.toString().toLowerCase().equals("firestrike")) {
			RS2Widget w = widgets.get(201, 1, 4);
			if (w != null && w.isVisible()) {
				w.interact("Fire Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("earthstrike")) {
			RS2Widget w = widgets.get(201, 1, 3);
			if (w != null && w.isVisible()) {
				w.interact("Earth Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("waterstrike")) {
			RS2Widget w = widgets.get(201, 1, 2);
			if (w != null && w.isVisible()) {
				w.interact("Water Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("airstrike")) {
			RS2Widget w = widgets.get(201, 1, 1);
			if (w != null && w.isVisible()) {
				w.interact("Wind Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
	}

Can be shortened to

    private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, spell.getSpriteIndex());
        if (spellWidget != null && spellWidget.isVisible()) {
            spellWidget.interact();
        }
        Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
    }

I would move some of the logic implementation in the loop to its own method for readability, so something like this:

    @Override
    public int onLoop() throws InterruptedException {
        dismissDialogue();
        if (!canAttack()) {
            this.stop();
            log("[Error] Out of runes.");
        }
        if (shouldAttack()) {
            if (shouldRotateScreen()) {
                rotateCameraToLesserDemon();
            } else {
                attack();
            }
        }
        return (random(600, 900)); // time before loop starts over
    }

 

Awesome, thank you for the tips. I'll definitely make those changes

2 hours ago, Nbacon said:

 ExperienceTracker class also look into that.

I did not know that existed haha. Thanks!

[Heist]

On 10/25/2020 at 1:39 PM, Hawk said:

You can significantly shorten some of the methods used while allowing for any other spells you wish to add in the future much more streamlined.

For instance,

	private boolean canAttack() throws InterruptedException {
		if(spell.toString().toLowerCase().equals("firestrike")) {
			return canUseFireStrike();
		}
		if(spell.toString().toLowerCase().equals("earthstrike")) {
			return canUseEarthStrike();
		}
		if(spell.toString().toLowerCase().equals("waterstrike")) {
			return canUseWaterStrike();
		}
		if(spell.toString().toLowerCase().equals("airstrike")) {
			return canUseAirStrike();
		}
		return false;
	}

Can be shortened if you initialize spell to a MagicSpell:

    private boolean canAttack() throws InterruptedException {
        return getMagic().canCast(spell, false);
    }

This

	private void selectSpell() throws InterruptedException {
		if (spell.toString().toLowerCase().equals("firestrike")) {
			RS2Widget w = widgets.get(201, 1, 4);
			if (w != null && w.isVisible()) {
				w.interact("Fire Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("earthstrike")) {
			RS2Widget w = widgets.get(201, 1, 3);
			if (w != null && w.isVisible()) {
				w.interact("Earth Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("waterstrike")) {
			RS2Widget w = widgets.get(201, 1, 2);
			if (w != null && w.isVisible()) {
				w.interact("Water Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
		
		if (spell.toString().toLowerCase().equals("airstrike")) {
			RS2Widget w = widgets.get(201, 1, 1);
			if (w != null && w.isVisible()) {
				w.interact("Wind Strike");
			}
			Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
		}
	}

Can be shortened to

    private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, spell.getSpriteIndex());
        if (spellWidget != null && spellWidget.isVisible()) {
            spellWidget.interact();
        }
        Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
    }

I would move some of the logic implementation in the loop to its own method for readability, so something like this:

    @Override
    public int onLoop() throws InterruptedException {
        dismissDialogue();
        if (!canAttack()) {
            this.stop();
            log("[Error] Out of runes.");
        }
        if (shouldAttack()) {
            if (shouldRotateScreen()) {
                rotateCameraToLesserDemon();
            } else {
                attack();
            }
        }
        return (random(600, 900)); // time before loop starts over
    }

 

Hey, I've been trying to implement this, but I can't figure out what you mean by "initialize spell to a MagicSpell". Could you elaborate a little bit on that?

[Hawk]

Quote

Hey, I've been trying to implement this, but I can't figure out what you mean by "initialize spell to a MagicSpell". Could you elaborate a little bit on that?

MagicSpell is an interface with several concrete implementations, one being Spells.NormalSpells: https://osbot.org/api/org/osbot/rs07/api/ui/Spells.NormalSpells.html

Enum classes have a method values() which returns an array of possible values. So you can loop through the values and return the appropriate NormalSpell by comparing the selected spell in your gui to the name() of the enum value. This should all be done within your GUI class.

Edited October 27, 2020 by Hawk

[Heist]

19 hours ago, Hawk said:

MagicSpell is an interface with several concrete implementations, one being Spells.NormalSpells: https://osbot.org/api/org/osbot/rs07/api/ui/Spells.NormalSpells.html

Enum classes have a method values() which returns an array of possible values. So you can loop through the values and return the appropriate NormalSpell by comparing the selected spell in your gui to the name() of the enum value. This should all be done within your GUI class.

Hmm, well i'm using widgets to set up the autocast, I don't know of any other way to do it. And with widgets you have to make sure the exact widget is selected for each different spell.

[Hawk]

4 hours ago, Heist said:

Hmm, well i'm using widgets to set up the autocast, I don't know of any other way to do it. And with widgets you have to make sure the exact widget is selected for each different spell.

If your spell strings are in a different format than the enum values you're going to have to modify one of the strings for comparison.

    String selectedSpell = "firestrike";
    public MagicSpell getSpell() {
        for (Spells.NormalSpells spell : Spells.NormalSpells.values()) {
            if (spell.name().replaceAll("_", "").equalsIgnoreCase(selectedSpell)) {
                return spell;
            }
        }
        return null;
    }

If your spell strings are in the same format as the enum values, you can simply perform a valueOf() check.

    String selectedSpell = "FIRE_STRIKE";
    public MagicSpell getSpell() {
        try {
            return Spells.NormalSpells.valueOf(selectedSpell);
        } catch (IllegalArgumentException e) {
            return null;
        }
    }

If you initialize your spell to a MagicSpell like above, this code should work, however I haven't tested it.

    private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, spell.getSpriteIndex());
        if (spellWidget != null && spellWidget.isVisible()) {
            spellWidget.interact();
            Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
        }
    }

Look at the fields a MagicSpell has: https://osbot.org/api/index.html?org/osbot/rs07/api/ui/MagicSpell.html
Look at the methods the Widgets class has: https://osbot.org/api/org/osbot/rs07/api/Widgets.html

The only methods of importance in this case are:

RS2Widget	getWidgetContainingSprite(int sprite) Checks all widgets (second and third level) for the specified sprite.
RS2Widget	getWidgetContainingSprite(int rootId, int sprite) Checks all widgets (second and third level) for the specified parent id (first level) for the specified sprite.

 The first one should work but it has a greater potential to produce a runtime logic error, so use the one specifying the rootId of where the sprite should exist instead.

[Heist]

On 10/28/2020 at 2:35 PM, Hawk said:

If your spell strings are in a different format than the enum values you're going to have to modify one of the strings for comparison.

    String selectedSpell = "firestrike";
    public MagicSpell getSpell() {
        for (Spells.NormalSpells spell : Spells.NormalSpells.values()) {
            if (spell.name().replaceAll("_", "").equalsIgnoreCase(selectedSpell)) {
                return spell;
            }
        }
        return null;
    }

If your spell strings are in the same format as the enum values, you can simply perform a valueOf() check.

    String selectedSpell = "FIRE_STRIKE";
    public MagicSpell getSpell() {
        try {
            return Spells.NormalSpells.valueOf(selectedSpell);
        } catch (IllegalArgumentException e) {
            return null;
        }
    }

If you initialize your spell to a MagicSpell like above, this code should work, however I haven't tested it.

    private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, spell.getSpriteIndex());
        if (spellWidget != null && spellWidget.isVisible()) {
            spellWidget.interact();
            Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
        }
    }

Look at the fields a MagicSpell has: https://osbot.org/api/index.html?org/osbot/rs07/api/ui/MagicSpell.html
Look at the methods the Widgets class has: https://osbot.org/api/org/osbot/rs07/api/Widgets.html

The only methods of importance in this case are:

RS2Widget	getWidgetContainingSprite(int sprite) Checks all widgets (second and third level) for the specified sprite.
RS2Widget	getWidgetContainingSprite(int rootId, int sprite) Checks all widgets (second and third level) for the specified parent id (first level) for the specified sprite.

 The first one should work but it has a greater potential to produce a runtime logic error, so use the one specifying the rootId of where the sprite should exist instead.

I see, thank you so much for that explanation. I have it all down, except the sprite index is returning the value of 71 when it should be 4. Any idea why? I can't seem to figure it out. And I don't want to use an alternate method such as color-picking the widget, because I think other spells use the same colors.

[Hawk]

18 minutes ago, Heist said:

I have it all down, except the sprite index is returning the value of 71 when it should be 4. Any idea why?

The sprite value of fire strike is 71. That method checks widgets with a root of 201 and has a sprite value of 71. The returned widget should be 201, 1, 4.

[Heist]

20 hours ago, Hawk said:

The sprite value of fire strike is 71. That method checks widgets with a root of 201 and has a sprite value of 71. The returned widget should be 201, 1, 4.

Yes sorry, I got confused. I meant the whole method returns null.

[Hawk]

4 hours ago, Heist said:

I meant the whole method returns null.

Can you show your code? It works for me.

[Heist]

3 hours ago, Hawk said:

Can you show your code? It works for me.

This should be everything related.

getSpell().getSpriteIndex() returns 71 like it should, but spellWidget returns null for some reason.

	private Spell spell;

	public void onStart() throws InterruptedException {
		spell = gui.getSelectedSpell();
	}
	
	public MagicSpell getSpell() {
        try {
            return Spells.NormalSpells.valueOf(spell.toString());
        } catch (IllegalArgumentException e) {
            return null;
        }
    }

	private boolean canCast() throws InterruptedException {
		return getMagic().canCast(Spells.NormalSpells.valueOf(spell.toString()), false);
	}

	private void selectSpell() throws InterruptedException {
        RS2Widget spellWidget = getWidgets().getWidgetContainingSprite(201, getSpell().getSpriteIndex());
        log("Widget: " + spellWidget + ".");
        log("Sprite index: " + getSpell().getSpriteIndex() + ".");
        if (spellWidget != null && spellWidget.isVisible()) {
        	log("Spell widget is visible.");
            spellWidget.interact();
            Sleep.sleepUntil(() -> widgets.isVisible(593), 5000);
        } else {
        	log("[Error] Spell widget not visible.");
        }
    }

 

Edited October 31, 2020 by Heist

[Hawk]

25 minutes ago, Heist said:

getSpell().getSpriteIndex() returns 71 like it should, but spellWidget returns null for some reason.

Are you ensuring you're on the spell select screen before trying to select the spell? Also, you should add a null check for the returned value of getSpell() since it has the possibility of being null.

[Heist]

13 hours ago, Hawk said:

Are you ensuring you're on the spell select screen before trying to select the spell? Also, you should add a null check for the returned value of getSpell() since it has the possibility of being null.

Yes, the spell select screen is open before trying to select the spell.

 

EDIT: I just discovered the actual sprite index should be 24, not 71. It works if I hardcode in 24 instead of the sprite index of getSpell().

Edited October 31, 2020 by Heist