ID:151930
 
Is it generally a good practice to split up giant procs into a group of smaller procs and call them separately? Because for my game, I have a big Death proc and it seems like there is a better way to handle it. This is because I take care of a lot of cleaning up the battle field and then giving players their rewards, etc. Thanks in advance.
Procs shouldn't generally span more than the vertical size of the screen, as it makes it harder to follow the logic therein. Also, if you're repeating a lot of the same operations among several procs, it's probably better to put those operations into a separate proc and call it from each proc.
The simple answer is that there is no answer.

The long answer is that it depends on what makes it easier to read.

If I break up a very long function into twenty smaller functions all distributed across five other files, my code becomes even more unreadable than if I just left things well enough alone. On the other hand, if the function has complex loops or routines that can be safely and simply exported into individual functions, the readability can improve significantly.
Well, here's a notion I subscribe to:

Each procedure performs 1 and only 1 function.

So as a for-instance, a Death proc handles the transition from a player being alive, to dead, and the consequences of that transition. So, what sub-actions need to happen for us to move a player from a state of alive, to dead?

Reduce their health/etc as appropriate.
Change the player's icon in some way.
Kill off any dependent mobs. (Units owned by the player, clones etc)

Now, the first two might be pretty easy, so no helper procedures for them. Killing off dependent mobs however could be tricky, so you might want a proc called say ... destroyUnits(), which finds units owned by the player and calls Death() on them as appropriate.

Rewards are an interesting one. You may be tempted to bundle that into Death() somehow. But let's say you add Civilian mobs, who people don't get rewarded for killing. If you're using a nice inheritance tree, presumably these Civilians are a sub-type of some mob type that has Death. Obviously you can't remove reward() from the Death() proc you are inheriting, so suddenly you need to rework things. But then of course, you might not add that.

The design flexibility comes from having reward() as it's own proc either way, seen as a small re-write because you didn't forsee something is unfortunate, but copy-pasting large sections of code or doing sweeping edits for the same reason is just a crime. Not to mention, it opens up the potential for easy additions, say you decide to add dueling, where-by a player only defeats another player, doesn't kill them, but still gets rewarded. That's a lot easier if reward() is a proc already.

Incidentally, don't be concerned much about 'performance' issues with small procs. I saw this banded about with Space Station 13's atmospheric system, where someone tried to claim that proc calls in the system would be less efficient than some straight code with a few if statements. It wasn't, it was a good order of magnitude more efficient to use some proc calls AND easier to read AND easier to modify (they sort've missed the point of what the procs were doing, which didn't help).
hmmm when it comes to procs, I try to keep them to a minimum, becasue massive ammounts of procs or large procs can cause major in-game lag when there are multiple players.
So I try to use Procs to a minimum.
In response to ElderKain
So what do you use instead of a proc? a verb?
In response to ElderKain
Procs that do silly things cause major in-game lag, like looping over really big amounts of data.
In response to A.T.H.K
A.T.H.K wrote:
So what do you use instead of a proc? a verb?

I still use procs, but not as much, also you don't have to have procs, like for example

I can use a area with the full info instead of using all the info with the use of a proc.
area/lrt
Entered(O)
usr.client:screen+= new/obj/hud/littleroottown/lrt01
usr.client:screen+= new/obj/hud/littleroottown/lrt02
usr.client:screen+= new/obj/hud/littleroottown/lrt03
sleep(20)
for(var/obj/hud/littleroottown/t in usr.client:screen)
del(t)
Exited(O)
for(var/obj/hud/littleroottown/t in usr.client:screen)
del(t)


I could use a proc and do it this way,
mob/proc
lrhud()
usr.client:screen+= new/obj/hud/littleroottown/lrt01
usr.client:screen+= new/obj/hud/littleroottown/lrt02
usr.client:screen+= new/obj/hud/littleroottown/lrt03
sleep(20)
for(var/obj/hud/littleroottown/t in usr.client:screen)
del(t)

area/lrt
Entered(O)
usr.lrhud()
Exited(O)
for(var/obj/hud/littleroottown/t in usr.client:screen)
del(t)


Note: for the above code there is more than 3 items in the lrhud(), I just shrunk it down to 3 for an example here, but there is a lot more to the code, lol

I can use procs also, it's just I try to keep them to a minimum and the above proc isn't much, it's just an example of that not all procs need to be in a proc.
In response to ElderKain
That code is very, very bad, just so you know.
In response to Popisfizzy
Popisfizzy wrote:
That code is very, very bad, just so you know.

personally, it may be bad, but it works so that's all that's fine with me ^.^
In response to ElderKain
It only works in the limited case you've but yourself in.
In response to ElderKain
Good job in making everyone think you're dumb, including me. >_>

All that talk about procs .. And then you post a snippet where you use the ENTERED() proc and the EXITED() proc. <_<
In response to Andre-g1
Well, he clearly didn't know what he's talking about to begin with; >_> the amount of procs or even their size (in number of statements, lines, bytes or whatever crap you'd care to think of) have nothing to directly do with the amount of lag produced by the game, there's no need to worry about that. There's also no such thing as 'avoiding procs', as actual game processing (ie, runtime DM code) has to be in procs (and verbs are also procs, you could think of them as a special [sub]type of proc), etc.
In response to Stephen001
Stephen001 wrote:
Incidentally, don't be concerned much about 'performance' issues with small procs. I saw this banded about with Space Station 13's atmospheric system, where someone tried to claim that proc calls in the system would be less efficient than some straight code with a few if statements. It wasn't, it was a good order of magnitude more efficient to use some proc calls AND easier to read AND easier to modify (they sort've missed the point of what the procs were doing, which didn't help).

Plus being easier to read is much better when you need to modify it because of bugs. If there are bugs in a program, and you have to sort through a giant proc, its much harder. For example, if the problem branches somewhere into the proc and just because the error is at line X doesnt mean that the error is actually caused from that location. Having smaller procs help you debug issues in those things because if you have parts of 10 possbily independant procs in one giant one, all the vars are defined in the beginning, etc the problem could branch to any part of the proc. Having smaller procs helps.
In my opinion, if it can be broken into smaller parts, do it as long as you arent transfering 50 arguments. (For example, dont get input in proc 1 and use the input in proc 2, thats just silly.)
In response to Polantaris
Good point. As always with these things, use at your discretion. It's just good to note that procedures exist to make your life easier, and are a language practice that has existed since the 1970's, with good reason.
In response to Popisfizzy
Perhaps you could explain the benefits of an alternative method?
In response to Stephen001
The most obvious one is that, were he to add wandering NPCs, or ones that follow other players, he wouldn't have errors coming out the ass when they enter the turf. He could also avoid the loop entirely by storing references to the objects and deleting them entirely (there'd still be a loop in a list storing that data, but it's getting to the data more directly). He could also make it easier to modify the proc (i.e., not having excessively many lines), by using lists and the newlist() proc.
In response to Stephen001
Thanks Stephen, that was very helpful. By the way, I already have a reward proc, but the death proc is still pretty big. Thanks to everyone else too.
Well, I usually split them up to do their main purpose. So, I might have one or two big procs that do some main thing (like determine damage and deal it), but even then, I split up some parts separately. For example, the damage proc also uses 10-20+ different procs to add in multipliers and attack effects, such as lowering stats or paralyzing them etc. Speaking of which, I really should split my Death check code up! It's pretty large, and has a little bit of repetitive code >_>. For death checking, though, I would only include killing the player. Then I would call another proc to clean the field, another to give rewards, etc.; what if I want to give someone rewards outside of battle, or something?
Often, splitting something up for me happens when I see one of two things:

1) I'm repeating something that I've also done elsewhere in exactly the same fashion, or one that is similar enough to generalize.

2) I have a need for some 'general case' procedures that do oft-requested functionality.

Heres an example of case 1:
mob/verb/Kick(mob/M in oview())
if(frozen)
src << "You're currently frozen"
return

src << "You kick [M]. How mean!"
M << "[src] kicks you. Kick \him back!"

mob/verb/Punch(mob/M in oview())
if(frozen)
src << "You're currently frozen"
return

src << "You punch [M], but he doesn't even seem to notice."
M << "The breeze picks up a little"


You'll notice that I do a 'check' in both verbs that ise exactly identical. Instead, I'd write a procedure to handle that:

mob/proc/IsReady()
if(frozen)
src << "You're currently frozen"
return 0
return 1


Then I change my verb to:
mob/verb/Punch(mob/M in oview())
if(!IsReady())
return

src << "You punch [M], but he doesn't even seem to notice."
M << "The breeze picks up a little"


This has two immediate advantages:

1) If you use the procedure in multiple places, you've just saved yourself a whole lot of hassle when adding more conditions to the player being ready.

2) You're seperating logic, so you can easily override IsReady() to add more behavior (in the case of libraries, f.ex) and more importantly: You only have to change one place to alter what makes a player ready for all the commands you use the proceduure in.

For more complex cases the line is rather blurry (as Jtgibson says); this is more a case of opinion. I tend to split up complex procedures when they get over a page, or if I notice repeated behavior that I hadn't thought of before. I don't really have much to show as an example, but heres a rather small one for case 2:

        Process(list/L)
if(!istype(L) || !("client" in L))
Log("Action 'playermove': Bad arguments supplied to Process ([list2params(L)])", EVENT_ERROR)
return 0

var/client/C = L["client"]
var/DBQuery/Q = mysql.CreateQuery()
if(istype(Q))
Q.Execute(SQLFormat_Select("clients", "*", "WHERE `key`='[C.key]'"))
if(!Q.RowCount())
Q.Execute(SQLFormat_Insert("clients", list("key" = C.key, "address" = world.address, "port" = world.port)))
else
Q.Execute(SQLFormat_Update("clients", list("address" = world.address, "port" = world.port), "WHERE `key`='[C.key]'"))
Q.Close()
del Q
return cluster.PlayerRecieved(C, L)


This procedure, without using any external procedure calls which I made myself, would look like this:

        Process(list/L)
if(!istype(L) || !("client" in L))
if(log_manager)
for(var/l in logs)
var/log/L = logs[l]
if((L.service_level <= log_manager.service_level) && L.log_list && (EVENT_ERROR in L.log_list))
var/ok = text2file(copytext(buffer,1,length(buffer)), "[log_dir_here][L.file_write].log")
. = 0
spawn while(!ok && . < 10)
sleep(rand(1,3))
ok = text2file(copytext(buffer,1,length(buffer)), "[log_dir_here][L.file_write].log")
.++
return 0

var/client/C = L["client"]
if(!mysql.dbcon)
dbcon = new()
var/ok = dbcon.Connect("dbi", "user", "pass")
if(!ok)
return

var/DBQuery/Q = dbcon.NewQuery()
if(istype(Q))
if(!mysql.queries) queries = new()
queries += Q
Q.Execute("SELECT * FROM `clients` WHERE `key`='[C.key]'")
if(!Q.RowCount())
Q.Execute("INSERT INTO `clients` (`key`,`port`) VALUES ('[C.key]','[world.port]')")
else
Q.Execute("UPDATE `clients` SET `address` = '[world.address]',`port` = '[world.port]' WHERE `key`='[C.key]'")
Q.Close()
del Q

var/mob/M = new()
M.name = C.key
M.gender = C.gender
if(("x" in L) && ("y" in L) && ("z" in L))
M.loc = locate(text2num(L["x"]),text2num(L["y"]),text2num(L["z"]))
M.key = C.key
C << output("You enter [world.name]", "main_output")
return M


As you can see, I have:

* A procedure to handle logging data
* A procedure to create new mysql queries and add them to a list, which also starts up the DB connection if its been severed.
* A few different procedures to generate general-purpose MySQL syntax, so that I don't mess up with typing errors.
* A procedure to recieve a player in this specific scenario (Not very general purpose, but this is from a library so in that case it serves to be overrided).