View Full Version : Writing Readable Code


Thalassicus
Feb 02, 2012, 07:07 PM
There's a basic tradeoff in programming:

http://forums.civfanatics.com/attachment.php?attachmentid=312866&stc=1&d=1328225161

As processor power rises and memory prices drop, the bottom two aspects of programming become less important. It's increasingly vital to create code that's easy to write, read, and maintain. I'm going to discuss some basics of the human factor here.



Why?
Guidelines
Further Details
Example



Why?

You should be able to come back to your work some time later and still understand what the heck you did. If your code looks like gibberish, you waste time figuring it out again.

For example, I've copied below some real code from a popular Civ 5 mod. Code like this is difficult to read. The variables have cryptic names, which are hard to understand and difficult to search for with find-replace tools. The formatting is also inconsistent. Some "if" statements are on multiple lines, while others are on one line. Writing code better than this makes it easier for you (and fellow modders!) to use or alter the code later.
if cT ~= "" then
repeat
local t1, t2; local more = false;
s, e, c, d = findToken( cT, e );
if s ~= nil then t1 = deserialize( c ); end
if d == "=" then
s, e, c, d = findToken( cT, e+2 );
if s ~= nil then t2 = deserialize( c ); end
end
if d == "," then e = e+2; more = true; end
if t2 ~= nil then r[t1] = t2;
else table.insert( r, t1 );
end
until e >= len and not more;
endGuidelines

These are two important aspects of writing readable code:

Use good formatting, with spaces and indentation. The earlier code example is cramped and hard to read.
Give variables descriptive names so they are instantly recognizable.

When formatting your code, it's a good practice to write code blocks like if-then-else statements on separate lines. Add extra lines between important segments of code. When writing complicated statements with multiple nested parenthesis, vary your spacing so it's easy to read like the example below. Inner variables are grouped together (centerY+maxRadius) while outer variables are spaced out:

((centerY+maxRadius) % mapH) or Constrain(0, centerY+maxRadius, mapH-1)The common naming standard in the game industry is camel case:

SomeFunctionName()
someVariableName
CONSTANT_VALUE

Join words (without spaces) into the name. Capitalize the first letter of functions, while leaving the first letter of variables lowercase. Constants are all caps with an _ underscore separating words.

Variable names are the most obvious part of Civ 5's code with poor design. Some parts of the code use camel case, while others use a variant called Hungarian notation (http://forums.civfanatics.com/en.wikipedia.org/wiki/Hungarian_notation). Pick one style and stick to it! This inconsistency makes it difficult to work with the vanilla Firaxis code. I'd recommend avoiding Hungarian notation in Lua/C++ for the following reasons:
Hungarian notation is a naming convention in which the name of a variable or function indicates its type or intended use.


When names are sufficiently descriptive, the additional type information can be redundant. E.g. firstName is most likely a string. So naming it sFirstName only adds clutter to the code.
If a variable's type is changed, either the decoration on the name of the variable will be inconsistent with the new type, or the variable's name must be changed.
Modern integrated development environments display variable types on demand, and automatically flag operations which use incompatible types, making the notation largely obsolete.
Compilers for languages providing type-checking ensure the usage of a variable is consistent with its type; checks by eye are redundant and subject to human error.


"Building the type of an object into names simply complicates and minimizes abstraction."
~ Bjarne Stroustrup, C++ creator

"Encoding the type of a function into the name (so-called Hungarian notation) is brain damaged—the compiler knows the types anyway and can check those, and it only confuses the programmer."
~ Linus Torvalds, Linux creatorWhen you read your code out-loud it should somewhat make sense. Variable names are usually a noun, and function names are a verb. These are the most common function name prefixes:


Do : perform some action without a return value
Is : returns a true/false condition
Get : retrieve a value
Set : store a value


Further Details


For further information about good coding practices, search Google for "coding standard" or check out the link below. Most standards of C++ also apply to similar languages like Lua and Java.

www.possibility.com/Cpp/CppCodingStandard.html (http://www.possibility.com/Cpp/CppCodingStandard.html)



Example

Below is an example of easily readible Civ 5 Lua code to get plots within a circular area. Variables names are descriptive, and tasks are separated with blank lines. The tasks are:


Initialize variables.
Check for world wrap.
Initialize variables dependent on world wrap.
Loop through each row and column of adjacent plots, and store plots with a distance between minRadius and maxRadius into plotList.

function GetPlotsInCircle(plot, minRadius, maxRadius)
local plotList = {}
local mapW, mapH = Map.GetGridSize()
local isWrapX = Map:IsWrapX()
local isWrapY = Map:IsWrapY()
local centerX = plot:GetX()
local centerY = plot:GetY()

x1 = isWrapX and ((centerX-maxRadius) % mapW) or Constrain(0, centerX-maxRadius, mapW-1)
x2 = isWrapX and ((centerX+maxRadius) % mapW) or Constrain(0, centerX+maxRadius, mapW-1)
y1 = isWrapY and ((centerY-maxRadius) % mapH) or Constrain(0, centerY-maxRadius, mapH-1)
y2 = isWrapY and ((centerY+maxRadius) % mapH) or Constrain(0, centerY+maxRadius, mapH-1)

local x = x1
local y = y1
local xStep = 0
local yStep = 0
local rectW = x2-x1
local rectH = y2-y1

if rectW < 0 then
rectW = rectW + mapW
end

if rectH < 0 then
rectH = rectH + mapH
end

local adjPlot = Map.GetPlot(x, y)
while (yStep < 1 + rectH) and adjPlot ~= nil do
while (xStep < 1 + rectW) and adjPlot ~= nil do
if IsBetween(minRadius, Map.PlotDistance(x, y, centerX, centerY), maxRadius) then
table.insert(plotList, adjPlot)
end

x = x + 1
x = isWrapX and (x % mapW) or x
xStep = xStep + 1
adjPlot = Map.GetPlot(x, y)
end
x = x1
y = y + 1
y = isWrapY and (y % mapH) or y
xStep = 0
yStep = yStep + 1
adjPlot = Map.GetPlot(x, y)
end

return plotList
end

Pazyryk
Feb 02, 2012, 07:54 PM
Great post! With 2000+ lines of Lua cod in my mod now, I'll add a few of my own:

Be consistent in your naming
Firaxis uses iPlayer / playerID and iBuilding / buildingID interchangeably, and then to add confusion they use "i" for things that aren't object IDs or table row IDs (following the Hungarian notation for integers, but not consistently). I use iUnit, iPlayer for game object IDs, and then buildingID, policyID, etc for table row IDs. All other variables get simple descriptive lower case. I'm not saying you should follow my rules. But it is worth the time to "formalize" your own before you get too far.

Don't pass functions around willy-nilly
Firaxis loves to do this. Not only does it make code incomprehensible to others, if you aren't very careful you will probably take a big performance hit (see item 8 here (http://trac.caspring.org/wiki/LuaPerformance)). Sure, sometimes it is the best solution to a problem. But think about alternatives.

Rules can be broken
Don't be too rigid. While I follow the normal separate line convention for if then blocks, there are several places in my code where I have dozens of lines like this:

if something then return false end
if something then return false end
if something then return false end
if something then return false end
if something then return false end
if something then return false end
if something then return false end
if something then return false end

That's less readable on separate lines. So bending rules sometimes helps, if there is a reason and the pattern is easy to see.

The_J
Feb 03, 2012, 07:02 AM
:thumbsup: to that 2 posts, both well thought.

As small side mark: The hungarian notation is probably a left over from Civ4, where it was also used, probably due to Python as language, to which 3 of the 4 things in the spoiler do not apply.
(but that's nearly offtopic now)

Thalassicus
Feb 03, 2012, 09:24 AM
Rules can be broken
Don't be too rigid. While I follow the normal separate line convention for if then blocks, there are several places in my code where I have dozens of lines like this:
I agree completely about this and the other tips you posted. That's why I consider these "guidelines" instead of rules. For one-line if statements, particularly when assigning a variable, I use the ternary "x and y or z" statement like in the earlier example:

x1 = isWrapX and ((centerX-maxRadius) % mapW) or Constrain(0, centerX-maxRadius, mapW-1)
x2 = isWrapX and ((centerX+maxRadius) % mapW) or Constrain(0, centerX+maxRadius, mapW-1)
y1 = isWrapY and ((centerY-maxRadius) % mapH) or Constrain(0, centerY-maxRadius, mapH-1)
y2 = isWrapY and ((centerY+maxRadius) % mapH) or Constrain(0, centerY+maxRadius, mapH-1)
In this situation it's not possible to directly replace the and-or statement with if-then-else. The code has to be written in a lengthier format with each x1/y2/etc variable name repeated twice:

if isWrapX then
x1 = (centerX-maxRadius) % mapW
x2 = (centerX+maxRadius) % mapW
else
x1 = Constrain(0, centerX-maxRadius, mapW-1)
x2 = Constrain(0, centerX+maxRadius, mapW-1)
end

if isWrapY then
y1 = (centerY-maxRadius) % mapH
y2 = (centerY+maxRadius) % mapH
else
y1 = Constrain(0, centerY-maxRadius, mapH-1)
y2 = Constrain(0, centerY+maxRadius, mapH-1)
end
While both styles are valid, in this circumstance I find it easier to read the compact grid-like form. It's also a habit from using the ?: operator in c++/java. Click Here (http://lua-users.org/wiki/TernaryOperator) for more information about ternary statements in lua.

Pazyryk
Feb 03, 2012, 04:21 PM
I started with the Hungarian system (a holdover from my limited Civ4 modding). I spent about 5 hours cleaning it out of my code some 6 months ago. I can honestly say that in the 10000 errors I've made and fixed over the last 6 months, not a single one was related to confusion in variable type.

(though I will still use "bDisable"... but only because it is short and informative)