Writing a simple relay script, feedback greatly appreciated.

Erich

Erich

Member
(Edit: I attached the file and updated the pastebin code, and also removed the stupid alt text.)

Long and short of it: After playing around with copy/paste experimenting thanks to Bale's topmenu.ash, I decided to try my hand at writing a relay script for myself not based on something else. The good news is, far as I can tell, the functionality is all there, and it runs pretty well. However, I have a feeling that it's done in a very sloppy fashion, and I was hoping to get a few pointers how I can improve it.

Like i said, it's a simple script:

Download into /relay folder: View attachment main.ash

Source: http://pastebin.com/9ahSTL6N

It pretty much adds buttons to the Daily Reminders! section of main.php that haven't been added by TPTB.

So... um... meow?

Thank you kindly for your time.
 
Last edited:
Erich

Erich

Member
Ok, I updated it a lot more, attached the script, and edited the first post. I want to stop doing that now until I get any feedback. Some specific questions would be:

* What's the best way to use is_unrestricted() for VIP clan items?

* I had problems with using > and <, but adding .to_int() fixed them. Am I doing that correctly?

* What's the best way to ask whether or not you have a telescope in your campground? I was thinking if(get_property("telescopeUpgrade") > 0), but I could be wrong.
 
H

heeheehee

Developer
Staff member
Erich said:
* What's the best way to use is_unrestricted() for VIP clan items?
Click to expand...
Best option is usually to check the base item (e.g. $item[Clan speakeasy]).

* I had problems with using > and <, but adding .to_int() fixed them. Am I doing that correctly?
Click to expand...
So, string comparisons are performed lexicographically -- i.e. compare the strings character by character (using character values), until you run into the end. Example: "21" < 3 returns true because 3 gets coerced to a string, and "2" is less than "3".

* What's the best way to ask whether or not you have a telescope in your campground? I was thinking if(get_property("telescopeUpgrade") > 0), but I could be wrong.
Click to expand...

Looks like it. I'd have guessed you'd want to use get_campground(), but it doesn't show up there.
 
Theraze

Theraze

Active member
1) No clue.
2) Yes. If you don't turn it into an int, you're comparing strings to see which is worth more. That's a rather bizarre check that will never give you the result you expect. Unless your expectations are really weird.
3) No. Remember point 2? You just failed to implement it. :) If you're comparing against an int, you need to turn your string (get_property) into an int first. Or you can compare against != "0" instead if you want to just say that anything except the string 0 is fine. Either way.
 
Erich

Erich

Member
Thank you kindly for the responses :)

heeheehee said:
Best option is usually to check the base item (e.g. $item[Clan speakeasy]).
Click to expand...

Makes sense. I'm guessing for the tree, I would use $item[crimbough]?

heeheehee said:
So, string comparisons are performed lexicographically -- i.e. compare the strings character by character (using character values), until you run into the end. Example: "21" < 3 returns true because 3 gets coerced to a string, and "2" is less than "3".
Click to expand...
Theraze said:
1)2) Yes. If you don't turn it into an int, you're comparing strings to see which is worth more. That's a rather bizarre check that will never give you the result you expect. Unless your expectations are really weird.

3) No. Remember point 2? You just failed to implement it. :) If you're comparing against an int, you need to turn your string (get_property) into an int first. Or you can compare against != "0" instead if you want to just say that anything except the string 0 is fine. Either way.
Click to expand...

Ok, cool, this makes sense.

heeheehee said:
Looks like it. I'd have guessed you'd want to use get_campground(), but it doesn't show up there.
Click to expand...

funny enough, I checked get_campground() first, expecting to see something like Telescope => 7. Do you think this is worth a feature request?
 
H

heeheehee

Developer
Staff member
- re: crimbough: yes.

- Telescope isn't an item; it'd have been $item[discount telescope warehouse certificate]. Looking at the code, I see that Mafia already tracks this, to some extent, but it doesn't expose this to ASH. I have a fairly simple change in mind, though.
 
xKiv

xKiv

Active member
Theraze said:
That's a rather bizarre check that will never give you the result you expect.
Click to expand...

Technically, it *can* give the expected results ... when you try it with numbers and "numbers" that have the same number of digits. So some people might check if it works, see that it "works", leave it without to_int, and then either be surprised that it doesn't work for inputs they didn't check, or make somebody else surprised that it works for all useful inputs.
It's still a good practice to explicitly convert before comparison. Not only will you get into a safer habit, your code will be more future-proof (what if jick decides that 5 more telescope upgrades are in order? etc ...)
 
Theraze

Theraze

Active member
xKiv said:
Technically, it *can* give the expected results ... when you try it with numbers and "numbers" that have the same number of digits. So some people might check if it works, see that it "works", leave it without to_int, and then either be surprised that it doesn't work for inputs they didn't check, or make somebody else surprised that it works for all useful inputs.
It's still a good practice to explicitly convert before comparison. Not only will you get into a safer habit, your code will be more future-proof (what if jick decides that 5 more telescope upgrades are in order? etc ...)
Click to expand...

True. Or what if mafia eventually disables implicit casts to help sloppy users sanitize their inputs? :)
 
Veracity

Veracity

Developer
Staff member
xKiv said:
(what if jick decides that 5 more telescope upgrades are in order? etc ...)
Click to expand...
Those of us who have at least 5 Discount Telescope Warehouse gift certificates in our DC will shrug and move on. Those who don't will either grind out more basement dives or will rage-quit.
 
Erich

Erich

Member
xKiv said:
So some people might check if it works, see that it "works", leave it without to_int, and then either be surprised that it doesn't work for inputs they didn't check, or make somebody else surprised that it works for all useful inputs.
Click to expand...

And this is exactly what happened to me :)

heeheehee said:
See r16839, lightly tested.
Click to expand...

Works fine on my end, thank you kindly:

if(get_campground() contains $item[2599] && get_property("telescopeLookedHigh") == "false")
 
Veracity

Veracity

Developer
Staff member
Don't use magic numbers. Instead of $item[2599], use $item[ Discount Telescope Warehouse gift certificate ].
 
Bale

Bale

Minion
Getting into a habit of identifying numbers by name is a good way to make your code indecipherable. The first time you had a reason to update that code you'd hate yourself for making it hard to read.
 
Theraze

Theraze

Active member
With the obvious exception being items where KoL has two identically named items, as per the quest items available through Edcore. Anytime KoL itself creates duplicate item names, your magic numbers will make you feel joy at avoiding their duplicative frustrations.

But if you do decide to use those numbers, definitely comment. And know that your script will break when KoL creates a new edition of the telescope certificate as part of that revamp rather than simply having you feel successful.
 
H

heeheehee

Developer
Staff member
Perhaps a bigger potential issue is KoL changing the item name and creating a new item with the old name (see: lead pipe).

IMO, it's better to simply do something like
Code: 
item TELESCOPE_CERT = $item[2599]; // commented if needed
 
Erich

Erich

Member
Thank you, thank you, and thank you. I think I'm pretty happy with it now. All I'm thinking of adding isn't really part of mafia as of yet (checks to see if you got the Witchess Buff or a piece of Floundry gear), so that can wait.
 
You must log in or register to reply here.
Top