Code Check

[andrewboss]

I would like to know if my code is proper or not, so criticism replies are allowed and much appreciated for improvement. If this could be done in a better way, please share your knowledge and led my life with advice  

	public boolean deposit_worn_items() throws Exception {
		RS2Widget deposit = script.getWidgets().get(BANK_PARENT, BANK_CHILD_DEPOSIT_BUTTON);
		if (deposit != null && deposit.isVisible())
			deposit.interact("Deposit inventory");
		new ConditionalSleep(5000, 500) {
			@Override
			public boolean condition() {
				return script.getEquipment().isEmpty();
			}
		}.sleep();
		return script.getEquipment().isEmpty();
	}

 

[GPSwap]

bank.depositWornItems();

is already on the api you should try using that instead

Edited March 24, 2018 by GPSwap

[Apaec]

Instead of using static ids, why not use the build in API functionality, i.e getBank().depositWornItems();

Be sure to check if the bank is open though!

Edited March 24, 2018 by Apaec

[Eagle Scripts]

13 minutes ago, andrewboss said:

I would like to know if my code is proper or not, so criticism replies are allowed and much appreciated for improvement. If this could be done in a better way, please share your knowledge and led my life with advice  

	public boolean deposit_worn_items() throws Exception {
		RS2Widget deposit = script.getWidgets().get(BANK_PARENT, BANK_CHILD_DEPOSIT_BUTTON);
		if (deposit != null && deposit.isVisible())
			deposit.interact("Deposit inventory");
		new ConditionalSleep(5000, 500) {
			@Override
			public boolean condition() {
				return script.getEquipment().isEmpty();
			}
		}.sleep();
		return script.getEquipment().isEmpty();
	}

 

First of all; your conventions are incorrect. Please make yourself known with the Java Conventions . Your method should be called depositWornItems.

The #interact() method has a boolean return-value which represents whether the interaction has been executed successfully or not. Try to make use of this feature by only executing the conditional sleep if you're sure that the widget#interact has been successfully excecuted.

Try to grab all your widgets dynamically instead of using static ids. You can do this by various things such as their actions, message, position, colour etc.

https://hastebin.com/niyohuboja.java

Edit: The API already has a method for this though, are you aware of this?

 

Edited March 24, 2018 by Eagle Scripts

[andrewboss]

2 minutes ago, GPSwap said:

bank.depositWornItems();

is already on the api

 

1 minute ago, Apaec said:

Instead of using static ids, why not use the build in API functionality, i.e getBank().depositWornItems();

Be sure to check if the bank is open though!

I read the API and I know that this function already exists. I just want to practice doing my own method and playing with it. So from above, does it look like good?

[andrewboss]

3 minutes ago, Eagle Scripts said:

First of all; your conventions are incorrect. Please make yourself known with the Java Conventions . Your method should be called depositWornItems.

the #interact() method has a boolean return-value which represents whether the interaction has been executed successfully or not. Try to make use of this feature by only executing the conditional sleep if you're sure that the widget#interact has been successfully excecuted.

Also try to grab all your widgets dynamically instead of using static ids. You can do this by various things such as their actions, message, position, colour etc.

 

Exactly what I was looking for, thanks!

Any guides where I can have some more information on how to get the widget dynamically?

[GPSwap]

4 minutes ago, andrewboss said:

Exactly what I was looking for, thanks!

Any guides where I can have some more information on how to get the widget dynamically?

 

[liverare]

public boolean depositWornItems() throws InterruptedException {

	RS2Widget deposit = script.getWidgets().get(BANK_PARENT, BANK_CHILD_DEPOSIT_BUTTON);
	
	ConditionalSleep conditionalSleep = new ConditionalSleep(5000, 500) {
		@Override
		public boolean condition() {
			return script.getEquipment().isEmpty();
		}
	};
	
	return deposit != null					// Is widget valid?
		&& deposit.isVisible()				// Is widget visible?
		&& deposit.interact("Deposit inventory")	// Did we click it?
		&& conditionalSleep.sleep();			// Have we deposited everything within 5 seconds?
}

Don't take my solution as correct, because there are many ways to do the same thing. However, what I like about the changes I've made include:

• Variables neatly defined.
• Sleep condition is an abstract class, which means you have to greedily use up multiple lines of code. Doing that and storing the object as a variable just makes things a little easier to read, than having mid-sections of code full of instantiated abstract classes.
• The return statement is structured so that everything, from the widget being valid, visible, and clicked, have to be true before we even get to testing whether or not we've successfully deposited our items within 5 seconds.
• ConditionalSleep#sleep function returns true if the items have been deposited prior to the time having been elapsed. This is particularly useful in laggy situations.

The only thing I dislike is that I've instantiated an object that may not even be used. What I'd normally do is create a separate class to handle the implementation of the conditional sleep, then I'd instantiate a new object from that class:

public boolean depositWornItems() throws InterruptedException {

	RS2Widget deposit = script.getWidgets().get(BANK_PARENT, BANK_CHILD_DEPOSIT_BUTTON);
	
	return deposit != null					// Is widget valid?
		&& deposit.isVisible()				// Is widget visible?
		&& deposit.interact("Deposit inventory")	// Did we click it?
		&& new SleepUntilDepositedItems().sleep();	// Have we deposited everything within 5 seconds?
}

private static class SleepUntilDepositedItems extends ConditionalSleep {

	public SleepUntilDepositedItems() {
		super(5000, 500);
	}
	
	@Override
	public boolean condition() {
		return script.getEquipment().isEmpty();
	}
}

It's all just preference, though.

Edited March 24, 2018 by liverare