[Starter] Script Improvement

[Shonx]

I know you probably get this a lot, But can anyone see anything that can be improved on my first script?

 

It's just a simple Nardah cooker that cooks Shrimps :3 atm, But i'm hoping to make it cook anything with a UI

 

http://pastebin.com/jyBCjaf7

 

Thanks for anything that contributes

Edited July 21, 2015 by Shonx

[Eliot]

I would suggest breaking things up into methods to make the code easier to read and maintain. I generally like a method to really only do one thing, of course sometimes that's not practical.

 

So, for example, in your state script I would do something like:

case COOK:
  cookFish();
  break;
case ...

I find this helps keep the code manageable for when projects get larger. That's just what I noticed right off the bat, there are other ways to structure scripts which I prefer over stateful scripts, but there is absolutely nothing wrong with a state script.

 

As a side note, I'd suggest reading about Java style guidelines as you're not following them properly. Perhaps also document your code.

 

Hope this helps a bit.

[frozen8]

Look clean to my eyes. Maybe add a couple random feature. Also your script way different. I don't use new String[] and stuff like that :P

Edited July 21, 2015 by frozen8

[bob10]

Cannot give much input (new as well), but could add random sleep lengths, walking paths? Would seem strange doing something exactly the same for a long period of time (personal opinion). Read over a thread that recommended getting the MethodProvider instead of directly typing it in:
 

this.localWalker.walk(this.bankArea, true);

to

getLocalWalker().walk(bankArea, true);

*Noticed you used sleep with random in a certain area within the script.

Nice work on starting script by the way.

Edited July 21, 2015 by bob10

[Shonx]

I would suggest breaking things up into methods to make the code easier to read and maintain. I generally like a method to really only do one thing, of course sometimes that's not practical.

 

So, for example, in your state script I would do something like:

case COOK:
  cookFish();
  break;
case ...

I find this helps keep the code manageable for when projects get larger. That's just what I noticed right off the bat, there are other ways to structure scripts which I prefer over stateful scripts, but there is absolutely nothing wrong with a state script.

 

As a side note, I'd suggest reading about Java style guidelines as you're not following them properly. Perhaps also document your code.

 

Hope this helps a bit.

Thanks for the feedback

I don't see how putting extra statements inside like: cookFish(); will help other than to help it read better

 

But i know i need to read up on learning more about Java

[Eliot]

Thanks for the feedback

I don't see how putting extra statements inside like: cookFish(); will help other than to help it read better

 

But i know i need to read up on learning more about Java

You're exactly right, it is to help it read better, it doesn't impact the performance of the script.

[Shonx]

You're exactly right, it is to help it read better, it doesn't impact the performance of the script.

Other than adding a GUI for choosing what you wanna cook, i don't need to be able to read the script (to put it bluntly).

 

So is there anything you can suggest for the script?

 

(sorry if i sound really rude that's not what i intended )

Edited July 21, 2015 by Shonx

[Khaleesi]

Other than adding a GUI for choosing what you wanna cook, i don't need to be able to read the script (to put it bluntly).

 

So is there anything you can suggest for the script?

 

(sorry if i sound really rude that's not what i intended )

 

Script looks good for first attempt!

 

Here is some feedback:

1) Sleep

Why are you using "sleep(500L)", ther eis no need to convert the number to a long.

Just use sleep(500). (Try to make sleeps random at all times)

 

2) Strange code

Not sure this code came as a decompiled exmaple or not,

"RS2Object bankBooth = (RS2Object)this.objects.closest(new String[] { "Bank booth" });"

- No need to cast a Gameobject ot a Gameobject.

- No need for a String array input

 

Simply do:

"RS2Object bankBooth = objects.closest( "Bank booth" );"

 

3) Nullpointers

getObjects().closest("Clay Oven").interact("Use"); 

This piece of code will return a nullpointer if no clay oven was found.

getObject.closest("Clay Oven"); can return null, if you try to interact with it in the meantime you'll get npe's.

 

Instead use:

Gameobject oven = getObjects().closest("Clay Oven");
if(oven != null)
  oven.interact("Use");

Always make sure to null check before interacting with something.

 

4)Java conventions

You need to learn some more about the java conventions (Java Rules)

An exmaple is your naming

Area cookArea = new Area(3432, 2889, 3435, 2886);

This should be:

private final Area COOKAREA = new Area(3432, 2889, 3435, 2886); 

Variables which do not changed shoudl be called 'final', also the whole variable name will be uppercase.

VaOther variables folow the "camelCase" principe. (Which you did good) 

This is not a big issue, and won't affect the script, but it will increase the readability fo the script

 

 

Don't get demotivated by the feedback, it's just some points to improve.

 

Always make your script readable... No matter what ^^

This is a small script and you won't have troubles finding code. (Atleast when writing it)

 

Some of my scripts have over 30 classes in them. (Try to make it more readable by using multiple classes)

If I wouldn't write it readable I wouldn't be able to modify/expand the script 1-2 months later.

At the moment you know perfectly how it works, but it will fade away slowly

 

Hope that helped ^^

 

Khaleesi

Edited July 22, 2015 by Khaleesi

[Shonx]

Script looks good for first attempt!

 

Here is some feedback:

1) Sleep

Why are you using "sleep(500L)", ther eis no need to convert the number to a long.

Just use sleep(500). (Try to make sleeps random at all times)

 

2) Strange code

Not sure this code came as a decompiled exmaple or not,

"RS2Object bankBooth = (RS2Object)this.objects.closest(new String[] { "Bank booth" });"

- No need to cast a Gameobject ot a Gameobject.

- No need for a String array input

 

Simply do:

"RS2Object bankBooth = objects.closest( "Bank booth" );"

 

3) Nullpointers

getObjects().closest("Clay Oven").interact("Use"); 

This piece of code will return a nullpointer if no clay oven was found.

getObject.closest("Clay Oven"); can return null, if you try to interact with it in the meantime you'll get npe's.

 

Instead use:

Gameobject oven = getObjects().closest("Clay Oven");
if(oven != null)
  oven.interact("Use");

Always make sure to null check before interacting with something.

 

4)Java conventions

You need to learn some more about the java conventions (Java Rules)

An exmaple is your naming

Area cookArea = new Area(3432, 2889, 3435, 2886);

This should be:

private final Area COOKAREA = new Area(3432, 2889, 3435, 2886); 

Variables which do not changed shoudl be called 'final', also the whole variable name will be uppercase.

VaOther variables folow the "camelCase" principe. (Which you did good) 

This is not a big issue, and won't affect the script, but it will increase the readability fo the script

 

 

Don't get demotivated by the feedback, it's just some points to improve.

 

Always make your script readable... No matter what ^^

This is a small script and you won't have troubles finding code. (Atleast when writing it)

 

Some of my scripts have over 30 classes in them. (Try to make it more readable by using multiple classes)

If I wouldn't write it readable I wouldn't be able to modify/expand the script 1-2 months later.

At the moment you know perfectly how it works, but it will fade away slowly

 

Hope that helped ^^

 

Khaleesi

Thank you for all the improvements

Yea ik i need to make it readable, but for my first script i don't think i'm gunna develop this any further,

 

So i'll take the making it readable into consideration when i move to my next script

[Bobrocket]

Thank you for all the improvements

Yea ik i need to make it readable, but for my first script i don't think i'm gunna develop this any further,

 

So i'll take the making it readable into consideration when i move to my next script

 

Honestly dude, you really need to get used to making readable code. I used to write code like you did, and honestly it really backfires in the future.

When I went on to make my AIO thieving script, I used the same method I did for every other script. This left me with 1000 or so lines of code in the onLoop alone (and this was before the script hit beta). It took me much longer to debug than it should.

When I moved that script to a more readable framework, everything was so much better. Even though I have more classes, I can debug quickly and I even got rid of a lot of code by doing so!

And don't be discouraged when your scripts get a lot of files, it only means progress For reference, here is what my AIO thieving script looks like (file wise):

I use 3 classes just to have a list of all the banks in RuneScape. I have classes which literally just reference methods in another file, too. But believe me when I say that this has helped.

 

Good luck, and maybe one day you'll be releasing scripts on the SDN!

[Gnomedesto]

looks good man, did you use any of the guides on osbot to help develop this? if so could you link it, i'm aiming to make my first script

[Khaleesi]

Honestly dude, you really need to get used to making readable code. I used to write code like you did, and honestly it really backfires in the future.

When I went on to make my AIO thieving script, I used the same method I did for every other script. This left me with 1000 or so lines of code in the onLoop alone (and this was before the script hit beta). It took me much longer to debug than it should.

When I moved that script to a more readable framework, everything was so much better. Even though I have more classes, I can debug quickly and I even got rid of a lot of code by doing so!

And don't be discouraged when your scripts get a lot of files, it only means progress For reference, here is what my AIO thieving script looks like (file wise):

I use 3 classes just to have a list of all the banks in RuneScape. I have classes which literally just reference methods in another file, too. But believe me when I say that this has helped.

 

Good luck, and maybe one day you'll be releasing scripts on the SDN!

 

Exactly! It's hard to write it perfeectly the first time though.

but the more you learn about how to make it readable the easier it will get

 

You'll find some way that fits with you and you'll be able to do lots of things a lot faster.

 

Here is an example of my pestcontrol script:

Edited July 22, 2015 by Khaleesi

[Shonx]

looks good man, did you use any of the guides on osbot to help develop this? if so could you link it, i'm aiming to make my first script

The only guide i used was this: 

http://osbot.org/forum/topic/60535-osbot-script-skeleton-the-minimum-code-to-make-a-script/

 

I used to code for a different bot (cant remember the name) That's why i've got the basic knowledge of coding.

[Bobrocket]

Exactly! It's hard to write it perfeectly the first time though.

but the more you learn about how to make it readable the easier it will get

 

You'll find some way that fits with you and you'll be able to do lots of things a lot faster.

 

Here is an exmaple of my runecrafting script: (Doesn't fit in 1 screen )

 

I'm not the only scripter that uses a pathfinder rather than a webwalker?

I think I can beat your file count pretty soon, I'll be adding around 100 files to my AIO thiever :p

[Twin]

Thank you for all the improvements

Yea ik i need to make it readable, but for my first script i don't think i'm gunna develop this any further,

 

So i'll take the making it readable into consideration when i move to my next script

 

Just trust me when I say, even if it's a small script, you need to gett in the habit of making it clean. I spent a week working on a script just to scrap it because I couldn't debug it due to the clutter. Was giving me a huge headache.