Best design practice?

[ZoPP0Qp5r9xM]

I am new to both programming and scripting. I was reading this tutorial on a Node System design and it sounded good (having events prioritized), but a couple users in the replies said it was overly complicated or not recommended/advised. I also looked at this old thread talking about concurrency. Other "designs" I am aware of is basic selection and switches, but I can foresee this limiting complexity. So I am wondering what exactly is the ideal way to structure a script, with OOP/polymorphism in mind?

I want to figure this out before even attempting to write scripts so that they don't end up being spaghetti.

[Apaec]

It's an interesting topic - there's no 'easy best solution' unfortunately!

The 'node' framework (or whatever people call it...) executes exactly the same as having plain if/else checks in your onLoop. There's really no benefit to it beyond organisation, but if your code takes up that much space then you're probably doing something else wrong!

The structure of your code very much depends on what script you are developing. You should come up with your own design patterns depending on what is appropriate, and attempt to model your problem with objects. Object orientation is very powerful and many OSRS automation problems can be well modelled with OO concepts - i'd suggest planning your structure as a diagram before beginning to write it!

Feel free to PM me with more questions should you have any, i'm always happy to help!

Apa

Edited December 27, 2017 by Apaec

[liverare]

The best design principle is: keep it simple.

If your abstracting shit out, having shit run concurrently, doing shit which--from the layman coder--looks completely alien, then you're doing it shitly. Right/wrong be damned. Keep it simple. You're writing a script, not a program. OSBot already abstracts out the three most important methods: start, loop, and exit. You don't need to further abstract shit unless you want to, and if you do, then you'd better be writing one hell of a program-script.

[aftabdear]

everything in onLoop pussy

[Septron]

I would avoid falling into the trap of passing the script instance into the class constructor when you can just handle it in the params for the method allowing for the use of interfaces.

https://gist.github.com/anonymous/86628d433f7f7cb444f20b4463c25d41

Edited December 27, 2017 by Septron

[Alek]

As @liverare stated, keeping it simple is usually the preferred route. The more you abstract code (i.e. node systems) , the more processing power is required. Now on the flip-side, a large script like an AIO agility script with multiple training methods and courses would be very messy to maintain all code in your onLoop. As an example in @Septron's snippet:

 

Quote

 

 tasks.add(new Task() {
            @Override
            public boolean isValid(Script client) {
                return client.getInventory().getAmount("Lobster") > 3;
            }

            @Override
            public int execute(Script client) {
                Item item = client.getInventory().getItem("Lobster");
                if (item != null) { // This should never happen.
                    item.interact("Drop");
                }
                return 1000;
            }
        });

 

The two lines in red are a redundancy, which is very hard to avoid using a node script. This can be solved by something like:

Item item = client.getInventory().getItem("Lobster");
if(item != null && item.getAmount() > 3) {
item.interact("Drop");
}

Although performance in this situation is VERY negligible, and may be entirely removed during the compiler optimization process, its still something to be mindful of. That's not to say node systems are bad, just understand that you must use the right tool for the right job.

[Septron]

While this doesn't solve the problem pointed out in Alek's post, this might be a good reason to keep them.

https://gist.github.com/48df1acb2e1de82f52d8e9877664efec

Edited December 27, 2017 by Septron

[liverare]

1 hour ago, Alek said:

As @liverare stated, keeping it simple is usually the preferred route. The more you abstract code (i.e. node systems) , the more processing power is required. Now on the flip-side, a large script like an AIO agility script with multiple training methods and courses would be very messy to maintain all code in your onLoop. As an example in @Septron's snippet:

 

The two lines in red are a redundancy, which is very hard to avoid using a node script. This can be solved by something like:

Item item = client.getInventory().getItem("Lobster");
if(item != null && item.getAmount() > 3) {
item.interact("Drop");
}

Although performance in this situation is VERY negligible, and may be entirely removed during the compiler optimization process, its still something to be mindful of. That's not to say node systems are bad, just understand that you must use the right tool for the right job.

Maintainability is the most important key of any piece of script or program, because it will break and some poor fucker's going to be tasked with fixing it. The simpler the script is (i.e., keep everything on the main loop), the easier time that guy's gonna have.

The way I'm going about things; I am doing everything I possibly can to steer clear from any kind of node/state system. Instead, if there's a scenario in which I need to (a large AIO agility script), then it's time to step back, break it down into multiple scripts and release them separately. The user gets to choose what's actually relevant to them (because an AIO with a high price tag might be mostly useless), and should one script from the batch break, then you only need to fix the problems unique to that script.

[Septron]

I would have to disagree, something like this is not acceptable in my book.

[liverare]

3 minutes ago, Septron said:

I would have to disagree, something like this is not acceptable in my book.

It's not pretty, but if you CTRL+F for the main loop method, then you can see what the script's actually doing:

	@Override
	public int loop() {
		if (!authCheck)
			return -1;
		if (!verified) {
			threadPitch(100);
		} else if (game.isLoggedIn()) {
			mouse.setSpeed(random(5, 8));
			if (inDungeon) {
				getCurrentRoom();
				if (exit) {
					exitDungeon();
					exit = false;
				} else if (finish) {
					finishDungeon();
					finish = false;
				} else if (newDungeon) {
					startDungeon();
					newDungeon = false;
				} else if (settingsFinished) {
					if (bossFight) {
						safeTile = null;
						if (getObjInRoom(FINISHED_LADDERS) != null) {
							finish = true;
						} else if (floor == Floor.FROZEN) {
							frozenBoss();
						} else if (floor == Floor.ABANDONED) {
							abandonedBoss();
						} else if (floor == Floor.FURNISHED) {
							furnishedBoss();
						} else if (floor == Floor.OCCULT) {
							occultBoss();
						} else if (floor == Floor.WARPED) {
							warpedBoss();
						}
						bossFight = false;
						retrace = false;
					} else if (explore) {
						exploreRoom();
						explore = false;
					} else if (open) {
						if (openNextDoor()) {
							explore = true;
						} else retrace = true;
						nearDoor = null;
						nearDoor2 = null;
						doorTimer = null;
						open = false;
					} else if (retrace) {
						retraceDungeon();
						retrace = false;
					} else {
						if (failSafe())
							return 100;
						if (developer)
							secondaryStatus = "We don't know what to do :(";
						return random(500, 1000);
					}
				}
			} else {
				if (newDungeon && inDungeon()) {
					status = "Starting a new dungeon!";
					inDungeon = true;
				} else if (objects.getNearest(END_STAIRS) != null) {
					status = "Jumping down the stairs...";
					if (doObjAction(objects.getNearest(END_STAIRS), "Jump-down")) {
						if (waitToStop(true))
							waitToStop(true);
					}
				} else if (objects.getNearest(ENTRANCE) != null) {
					enterDungeon();
				} else {
					// Failsafe
				}
			}
		}
		secondaryStatus = "";
		unreachable = false;
		return random(0, 20);
	}

What's wrong with this script is that there are a lot of functions which could be stored in their own classes, such as:

private boolean walkFast(RSTile dest, final int r)
private boolean walkTo(final RSTile dest, final int r)
private boolean walkToScreen(final RSTile dest)
private boolean walkToMap(final RSTile t, int r)
private boolean walkToDoor(final int r)
private boolean walkBlockedTile(RSTile dest, final int r)
private boolean walkAround(final RSTile t, final int x, final int y, final boolean isNpc)
private void walkAdjacentTo(final RSTile dest, final int maxDist)
private boolean waitToStop(boolean deanimate)
private boolean useItem(final int itemID, final RSObject obj)
private boolean useItem(final int itemID, final RSNPC npc)

That's just some (because the list is huge! Just copy/paste that code into Notepad++, hit Language > Java, then View > Fold All).

That being said, I have no right to criticise one of the best scripts in RS history (one that I used myself). The developer of this script was a genius.

[ZoPP0Qp5r9xM]

4 hours ago, Septron said:

I would avoid falling into the trap of passing the script instance into the class constructor when you can just handle it in the params for the method allowing for the use of interfaces.

https://gist.github.com/anonymous/86628d433f7f7cb444f20b4463c25d41

 

10 hours ago, liverare said:

The best design principle is: keep it simple.

If your abstracting shit out, having shit run concurrently, doing shit which--from the layman coder--looks completely alien, then you're doing it shitly. Right/wrong be damned. Keep it simple. You're writing a script, not a program. OSBot already abstracts out the three most important methods: start, loop, and exit. You don't need to further abstract shit unless you want to, and if you do, then you'd better be writing one hell of a program-script.

 

Thanks for the replies. I think I will start simple and then only if I find things are getting unwieldy I will see if I can abstract.

[Butters]

Very interesting to see that most of the experienced scripters, who I believe wrote preety complex scripts don't fancy the Node or Task based designs. Though in my mind it is true that for simple scripts there's no need for it, but if you're making anything that might have the word "progressive" or needs to scale eventually - I don't see how liverare's example would make my life easier. What I love about the node design that, if done properly, it scales easily and most of the classes are reusable.

[Explv]

On 27/12/2017 at 8:33 PM, Septron said:

I would have to disagree, something like this is not acceptable in my book.

 

On 27/12/2017 at 8:42 PM, liverare said:

That being said, I have no right to criticise one of the best scripts in RS history (one that I used myself). The developer of this script was a genius.

 

Dear god that script makes me throw up in my mouth every time I see it.

Wouldn't really call him a genius. Sure, he made a popular bug-free script, but he could certainly use a lesson or two on code design.

Edited December 30, 2017 by Explv

[Shudsy]

Jesus christ that dungeoneering script is large