Jump to content
Sign in to follow this  
andrewboss

Code Check

Recommended Posts

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

 

Share this post


Link to post
Share on other sites
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 by Eagle Scripts
  • Like 2

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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?

  • Like 2

Share this post


Link to post
Share on other sites
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 by liverare
  • Like 3

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Sign in to follow this  

  • Recently Browsing   0 members

    No registered users viewing this page.

×
×
  • Create New...