GreatFisher's Site - My first library.

My first library. in Off Topic

ID:122907

Jan 17 2012, 1:41 pm

Came up with this very convenient proc will coding Faymoor and thought other people might get some use from it.

Wasn't to sure if there was anything out there like this and could be asked to look lol. So here it is,http://www.byond.com/developer/GreatFisher/Path_setLibrary, the Path_set library. It lets you set a mobs movement path through a list var containing the letters N,S,W and E.(it can contain other letters but if they are there they will be filtered out)

That's all I can think of telling you that's not in the read me so please, check it out and give some feedback. Note I will only update if I need to or if there is a high demand for it.

P.s. I know this isn't my best written post but I was rushing and wasn't paying a lot of attention.

#1 Jan 17 2012, 2:28 pm (Edited on Jan 17 2012, 2:33 pm)

DarkCampainger

First of all, congrats on your first library. Here's some constructive criticism for you:

When writing code, always consider the DRY principle: Don't Repeat Yourself. Looking at your Path_set() process, 90% of the code is repeated for each of the directions. The only thing that varies is the direction passed into step().

There is also no reason to be using goto in this code, a while() or even a for() loop would be trivial to implement and much cleaner.

Instead of storing a character (N, S, E, W), you could store the actual direction in the list (NORTH, SOUTH, EAST, WEST, NORTHEAST, NORTHWEST, ect). Then you could just pass that value directly to step() and not need a chain of if() statements.

It's unnecessary to strip out invalid values from the sequence list when only valid values have any actions (and if you switch to using the direction constants instead of strings, step() will handle any validation for you)

If you're passing the target mob as an argument, it doesn't make sense to also have the process defined under /mob (in other words, both M and src are being set, when you only need M). I would suggest just making it a global process like the built-in step().

Your process should work for any atom/movable, so it's misleading to define the argument as a mob.

This is a personal preference of mine, but if you're going to implement a feature as game-specific as randomized delays for movement between tiles, I would make an argument to control (or disable) it. Something like delayRange.

If you're interested in implementing any of these suggestions and would like a more thorough explanation, just let me know. Also, don't let my criticisms devalue your achievement. Releasing a library is a big step. My only goal is to help you make it even better.

#2 Jan 17 2012, 2:31 pm
GreatFisher	Thanks for the criticism and re-enforcing the fact that I knew it was pointless releasing this lol. The second part refers to the fact that any old fool could put this together in a matter of minutes and so it is redundant.

#3 Jan 17 2012, 2:36 pm

DarkCampainger

GreatFisher wrote:

Thanks for the criticism and re-enforcing the fact that I knew it was pointless releasing this lol. The second part refers to the fact that any old fool could put this together in a matter of minutes and so it is redundant.

Well, any experienced developer could write this themselves, that's true. But I would say that the majority of BYOND developers are far from that point, and might implement it in the way you did, or maybe not be able to at all. If you can consider and implement my suggestions, it will be better than the average BYONDer could implement themselves, making it a useful library or demo. I would lean more towards demo because of how short it is.

#4 Jan 17 2012, 2:37 pm
GreatFisher	I am recoding it as we speak lol. with the whole toggleable random interval and an added loop feature may also add in all 8 directions.

#5 Jan 17 2012, 3:28 pm
GreatFisher	Okay it is updated give that a go, hope you like the improvements, I kept the check for the validity of a direction just for safety purposes, it did some funny things without it lol.

#6 Jan 17 2012, 4:45 pm (Edited on Jan 17 2012, 4:50 pm)

DarkCampainger

Much better, and definitely much shorter. I do have some more comments for you, though :)

----------------------------------------

For variables that are expected to be either TRUE or FALSE (such as your loop and Rand_step_interval arguments) you should use boolean-conditionals instead of checking for specific values. This makes the code more robust, because it can handle unexpected values more gracefully. Some people also find it easier to read, and it hints at the usage of the variable (in other words, it suggests it's a boolean true/false variable, instead of a normal numeric value)

So instead of if(Rand_step_interval == 1) you could do if(Rand_step_interval). To check if the value is false, you would do if(!Rand_step_interval). This applies to loop conditionals as well.

----------------------------------------

You should never need to use goto. There are only a handful of rare circumstances where it's necessary, and I've never found myself in one. The issue with goto is that its logical flow is backwards. When you read a process, you read it from top to bottom. When the author of that process uses goto, the first part of the goto-loop you hit is a label. Labels offer very little information--only what you can gather from its name. You have no idea what section of code is being looped, nor do you know the condition under which it is being looped. It's not until later in the code that you hit the goto statement which reveals the conditions for the loop and the section of code being looped. This makes code a bit more difficult to read.

Now consider the situation where you have multiple labels and goto statements, maybe even multiple goto statements for each label. This is commonly referred to as "spaghetti" code, because like a plate of spaghetti, it's nearly impossible to follow the flow of the lines without pulling everything apart.

Instead of goto, you should use while(), for(), and do while() loops. Using these structures instead of goto has several key advantages (mostly in regards to readability). You immediately know the condition of the loop, the tabbing immediately tells you the exact code being looped, and any nested loops (loops within a loop) are cleanly separated.

In your case, you can use a do ... while() loop to make the code always run at least once, and then loop only if the 'loop' argument is set.

Example:

do
    // Looped code here. Runs once, then checks conditional in while() statement to see if it should run again.
while(loop)

You can also see the boolean check mentioned above in the while() conditional.

----------------------------------------

You can get buggy behavior when you alter a list of objects that you are currently looping through (eg path -= D). I'm guessing that's where the "funny things" was coming from without the validation. There's also no need to remove the direction from the path list, because the for() loop will automatically advance to the next direction. Also, because directions are just numbers, removing one from the list doesn't make it clear which one is being removed, so it could just as easily be removing a direction that hasn't been processed yet. With that fixed, you should be able to get rid of the validation step.

----------------------------------------

When making a library, it's important to consider how your code will integrate with other projects. Currently, you've included some basic world settings in your library:

world
    fps = 25
    icon_size = 32

    view = 6

mob
    step_size = 8

Because this code isn't related to your library's functionality, it should be removed. As it is, it will just cause problems for people who try to use your library.

----------------------------------------

Demos are important for a library. A running example is often more useful to someone who wants to learn to use your library than a textual explanation. Being able to actually see the effect your library will have will also make people more likely to use it.

To create a demo within a library, simply create separate code, icon, and map files that implement your library. Then make sure they are NOT included in the environment (they should be unchecked). When you go to package the source files, enter the names of you map and code files in the "Demo Files" box.

When done correctly, you can run the demo from Dream Maker by simply compiling and running the library. But, when a user includes your library in their game, the code and map files you specified as "Demo Files" won't be used.

----------------------------------------

So, there are still a few improvements you can make if you're interested. Some are important (not included rogue code in libraries, for example), other more style related.

Anyways, good luck. Keep on programming, and you'll start to pick up all these little details as second nature :)

#7 Jan 17 2012, 4:54 pm (Edited on Jan 17 2012, 5:02 pm)
GreatFisher	Wow that was wordy but definitely helpful thanks, I'll amend said things to the best of my ability. The world values are kinda relevant because the distance value only works with that but I'll just specify that lol. I don't really have time to make a demo so I'll put one together when I have time.

#8 Jan 17 2012, 5:03 pm
GreatFisher	Okay hows that I think I corrected everything you said about.