Log in

DotP 2014: Cleaning Code

Clean Code

Creating clean code is the difference between code that you can fix later when something needs changed and code that you have to rewrite from scratch. It's also the difference between code that you need to fix because there are simple errors and code that you never need to look at again. Coding cleanly isn't a new concept at all, but here are some things to keep in mind as you go.

Whenever you make a variable, give it a useful name. Often, it's okay to simply name a target pointer "target", but if you've got two targets, or you'll be dealing with lots of pointers, then you'd be much better off naming them something a little more descriptive. If they're interchangeable, then this isn't as important, such as with the ability "destroy two target creatures". But, if the ability is something like The Mimeoplasm, where you've got two different pointers to creature cards, and the pointers will do different things, naming them differently is a good idea. If they're different types, such as a player and a creature, then name them "oPlayer" and "oCreature" or "oTargetPlayer" and "oTargetCreature".

You'll notice I add "o" before a lot of variables. Other times it's "i", or something like that. These prefixes denote the type of data the variable contains. "o" is for object, and "i" is for int. You'll also see "s" for string, "b" for boolean, and filters are variously "f" or "o" (they are, internally, objects; i.e., something you can call functions on). Data chests also get "o", because, again, they're internally objects, since you can call functions on them. (When I say "call functions on them, I mean EffectDC():Get_Int(0). The colon after EffectDC() means Get_Int(0) is being called on the data chest returned by EffectDC()). Finally, in some functions, you'll see "a" as a prefix, usually with another prefix as well. That's because "a" is for "array", which is a list. So, the other prefix tells you what the list is a list of. "aiSomeVar" is an array of integers.

OBSCURE NOTE: Technically, Lua's arrays aren't really "arrays" in the traditional sense. They're all tables. These are actually a bit more versatile. If you're familiar with arrays from other programming languages, Lua's arrays aren't fixed in size.

This is a really good habit to get into, and it'll allow you to more easily recognize what a variable contains without having to backtrace where it came from. When you're coding, you might not think it'll matter much since you can easily remember where the variable came from, but it's also very important for other coders who may come in to fix the code at some point. With clean code, it's much easier for them to fix it and takes a lot less time. It also means they're less likely to need to.

Another thing to keep in mind is to use a function call whenever possible to eliminate large sections of repetetive code. This isn't always viable, but it's often applicable. For instance, consider this code, which is on many cards.

<AUTO_SKIP>
 local effectController = EffectController()
 if effectController:GetTeam():IsSharedLifeTotal() == true then
  if effectController:GetLifeTotal() >= 30 then
   return true
  end
 else
  if effectController:GetLifeTotal() >= 20 then
   return true
  end
 end
 return false
</AUTO_SKIP>

If you take the time to parse through this code, it simply returns true if your current life total is >= to your starting life total. This can be accomplished with a single line of code, which is not only much easier to read and understand, but can also be updated in the future on every card that uses it (such as when Commander was introduced).

return EffectController():GetLifeTotal() >= CW_General_GetStartingLifeTotal()

Now, it's very easy to read (especially once you get used to reading "CW_General_" as a prefix and not part of the function name).

For the record, IsSharedLifeTotal() is one way to tell if you're in a two-headed giant game.

Anyway, now that EDH is out, the starting life total has changed for 2, 3, and 4 player games. In 2 player games, it's now 30. In 3 and 4 player games, it's now 40. But all of those cards need to be updated to reflect that. When code is repeated too often on different cards in ways like this, it makes it very hard to change things in the future. In order to change one thing, you have to edit hundreds of cards.

Error Resistance

One way to help prevent bugs is to make sure that while you're coding, you're accounting things that might go wrong. Consider the very simple ability "Tap target creature." Here's the basic code for this ability.

<TARGET tag="CARD_QUERY_CHOOSE_A_CREATURE_TO_DISPATCH" definition="0" compartment="0" count="1" />
<TARGET_DEFINITION id="0">
 local oFilter = ClearFilter()
 oFilter:Add(FE_TYPE, OP_IS, CARD_TYPE_CREATURE)
</TARGET_DEFINITION>
<RESOLUTION_TIME_ACTION>
 EffectDC():Get_Targets(0):Get_CardPtr(0):Tap()
</RESOLUTION_TIME_ACTION>

Within this code, there's a serious flaw. The issue here is that the creature's pointer is stored when the spell is played, but the pointer is accessed when it resolves. Between those two times, the creature's pointer might become invalid, such as when the creature is destroyed.

If this happens, then the game will continue to run normally, but upon exiting, it'll show an error log. In order to protect against this, you should make it a habit to always check if card or player pointers are nil before trying to do anything with them. To do this, you simply compare them to nil. If they aren't nil, then they should be safe to interact with.

<TARGET tag="CARD_QUERY_CHOOSE_A_CREATURE_TO_DISPATCH" definition="0" compartment="0" count="1" />
<TARGET_DEFINITION id="0">
 local oFilter = ClearFilter()
 oFilter:Add( FE_TYPE, OP_IS, CARD_TYPE_CREATURE )
</TARGET_DEFINITION>
<RESOLUTION_TIME_ACTION>
 local oTarget = EffectDC():Get_Targets(0):Get_CardPtr(0)
 if oTarget ~= nil then
  oTarget:Tap()
 end
</RESOLUTION_TIME_ACTION>

In this code, It gets the target chest, and immediately gets the card pointer from that chest. Since the chest is valid (even if all targets are destroyed, the chest itself is still intact), there's no error so far. On the next line, it checks "if oTarget ~= nil then" ("~=" means "not equal" in Lua). So, Before trying to interact with the card, it's first validated.

There is still the potential that the chest itself is nil for some reason. If this happens, then you'll again get an error. This is very rare, but to combat it, you'll often seen a line like this.

local oTarget = EffectDC():Get_Targets(0) and EffectDC():Get_Targets(0):Get_CardPtr(0)

This oTarget becomes either the card pointer or nil. It never throws an error. This is because it evaluates from left to right. On the rare occasion when EffectDC():Get_Targets(0) is nil, the "and" syntax makes it stop evaluating, and assigns the current value (nil) to the variable. If the chest isn't nil, then oTarget becomes Get_CardPtr(0), which might still be nil, but that's checked on the next line. Unless EffectDC() itself somehow becomes nil (which virtually never happens), it'll never throw an error. It'll always run correctly with no scary warnings when the game closes.