I've seen my fair share
of hacky code, hell I've even written up stuff that now makes me want
to puke to death and hide myself from the world. (Read: Any code
written prior to two months ago.)
This master-failure, however, does
not come from me (thankfully). Rather it's from a community extension
to a game called “Garrysmod” (the author is so humble indeed).
Now, you might say the
following:
“Bah, it's a 3rd
party fan extension, it's alright if it's a bit hacky.”
- Or -
“Give the author a
break, he's trying to make something neat.”
I'd agree in most cases,
but this one is almost on relative par with Codethulhu.
Start things off, let's
put the following code in context. The objective is to make a
'microwave rifle' that makes NPC's head swell up and explode. (It can
also throw 'microwave grenades'.) Personally I found this a hilarious
little time waster until I started looking at the code. First red
flag went up when I saw this:
That's right, he put all
the weapon code into a single file. Most Scripted WEaPons
(SWEPs, I didn't choose these names and I'm not making is up)are divided into three files:
cl_init.lua: Anything
that has to do with client-side stuff (fx, etc..)
shared.lua: Anything
that “needs to be shared both by the server and client”
init.lua: Anything
that has to do with server-side stuff (synchronization, etc..)
So apparently the entire
thing needs to be done both on the client and the server. Humm,
strange, but ok, if that's what it takes. I was perplexed but
lenient, but soon any forgiveness that was in my mind quickly
vanished. I'd try to put this one into words, but words are
inadequate, so I'll just show you and explain under each snippet (in
case you don't know Lua).
Exhibit A (Or “What's
scope indenting?):
I know it's very hard,
if at all possible, to read the actual code, but that is not the
point of this exhibit. If anyone can decode a rational scheme of
indentation in this picture, please feel free to email me:
evilpineapple <atz> cox <d0tz> n3t (If you're human you
should be able to decode this, no spaces.)
Exhibit B (Or, “Rude
Goldberg Style”):
Rudey would be proud of
the author, he goes through all the pain of creating a custom entity
type (another file, 1/1/2 kb because it's effectively empty) so he
can do all the processing of a 'microwave grenade' in the gun entity
code instead of in the grenade entity code. To make this quick let me
present a condensed list of these worse-then-failures present in the
picture above:
CurTime() Spam: Repeated
call to C function when he could have stashed the value as a local
table.Count(Target):
Counts a target... So he can loop 1 to N... When there is a dedicated
function (pairs, or ipairs) for table iteration... Right...
string.find(Offer:GetClass(),
"npc_*"): This guy will haunt us into exhibit C
Fun Side Effect: The
grenade mysteriously stops working when the player dies and the gun
entity is removed.
Exhibit C (Or “The
If-Then Jungle”):
Now you obviously can't
read this, but each of those randomly indented sections of code
consists of:
Once again, let me
condense this:
'Offer:GetClass()' : He
calls this 53 times instead of calling it once and stashing it to a
local
string.find(): He then
proceeds to call a string search operation for the exact entity
names he wants to filter for. Obviously it's better to search for
npc_antlion and then for npc_antlion_worker then to just search for
antlion and finish it there.
Total lack of possibly
offending elseifs: If a string search succeeds for one entity name,
apparently it's better to keep searching all the others anyways, just
in case GetClass changes it's return value in the middle of a Lua
callback.
Obviously either the
author was naïve (an idiot) or I'm missing something blindly
obvious. Note that he could have also coded it as a table with string
indexes corresponding to the entity values he wanted. If he'd done
that, 200 lines could be condensed into:
local specs =
EntitySpecifications[Offer:GetClass()]
And bailed out if it was
nil. Please also note he does a search for “npc_*” prior to doing
all of these ifs. Obviously that was too easy a solution or
something.
Fun Fact: Because “
npc_antlion” is a subset of “npc_antlion_worker” the second
string search will never be called if it is an antlion. I guess we
all get lucky sometimes.
Exhibit D (Or “LET'S
DO IT AGAIN!”):
Apparently unsatisfied
at his previous set of atrocities he proceeds to do the same again.
Remember that initial search for 'npc_*' I mentioned? Well,
regardless of if it is an NPC or not, we must check if it's a ragdoll
as well!
See Exhibit C's list of
atrocities. Substitute :GetClass() by :GetModel().
Exhibit E (Or “Just in
case, let's copy and paste it around.”):
Now that we're out of
the Think() function (that gets called every frame), let's move onto
PrimaryAttack().
Looks familiar? And a
bit farther down:
Yep, that's right. He
copied and pasted that blasphemous code from the Think() function
into PrimaryAttack() and added a line or two of code here and there.
I can't tell any more if it was supposed to be originally in
PrimaryAttack or in Think, but at this point I don't think it
matters.
See Exhibit C and D for
atrocities committed.
Exhibit F (Or “Why
bother with the custom entity I made, I'll just use prop_physics.”):
Remember when I rante-,
err, talked, about that grenade processing code in the gun entity and
how he should have put it in the grenade entity's code? Well, it
turns out he isn't even using it. Poor, littl' WaveGrenade never ever
gets used... How sad...
Interestingly enough he
uses all three lua files in it anyway...
Exhibit G (Or “There's
one thing done right. Or rather, there's one thing done right by
someone else.”):
This is the only elegant
looking piece of the whole thing. It's also the only one that he
didn't write (other then replacing the author & email field), so
everything works out in the end.
Conclusion (Or “Pray
he doesn't get hired by M$”):
There are other
atrocities I could point out, like how he spams fx entities and eats
up all the ParticleEmitters without releasing them (in a reasonable
amount of time). Or how he's said with a sad face that these errors
that people get (like 'Out of ParticleEmitters') aren't his fault,
that they're an engine issue and he can't fix them. Or how he keeps
state information for the gun in a global weapon table so the gun
cannot be effectively used by more then one player at once, etc...
But all of these pale before one simple fact:
He's
the “b3sT LUA God GM0d h@S Evah S33n!”
– Unnamed Follower of His
However!
There is
one absolutely redeeming fact:
He released a 'fix'.
And we call
all now rest safe in the knowledge that things always improve:
See! No
more string.finds! Of course, that doesn't bring much comfort,
especially in the light of this:
Your eyes
aren't lying to you, he really is iterating through all entities
present in the game, testing each one by one, to see if it's a
vehicle. There are about 1000 to 2000 entities in a game, and he
could just have done ents.FindByClass(“prop_vehicle_*”) instead.
The only thing I don't get is why he is testing a boolean to see if
it's true if the result of such a comparison is the value true. And
why he is using network variables on what should be a purely
server-side script...
PS: Anyone want to see
his Time-Grenade code? He's back to his old tricks!
Pardon any mistakes in spelling, it's midnight and I'm getting really annoyed at the fly who's buzzing in the room.