Did I hear a request for a code review?!? I love reviewing code. This may have been better posted in the code review section, but it unfortunately isn’t too popular right now. I’ll comment on it and see if it gets moved.
After I got my first job as a software tester at age 15, one of my first purchases was a book on software development best practices (I was an odd one, I’ll admit it…). I’d recommend it in addition to searching on the web because on this topic books aim to be a more comprehensive resource whereas most webpages aim to critique a small area of coding practices.
I’ll first start with some medium-level critiques, continue to the nitpicky personal preference ones, and finally the large picture.
Medium-level
- The constant’s table, is this passed anywhere? For constants that are only used in one script, it is idiomatic to place them near the top of the file as local variables. If it is used elsewhere, then I’d move it to another module to live alone in a dark corner and rededicate the main file to code. In general, try to avoid nested structure unless absolutely necessary to divide unlike groups of information.
- A common best practice is to name constants using ALL_UPPERCASE_WITH_UNDERSCORES. This makes the intention of not editing it clear.
- Functions in tables is generally avoided, especially with this deep of nesting! Most of a programmers time is spent staring at the leftmost 3 inches of their screen. It makes moving around code and exploring sections easy. Also with this deep of nesting, it is likely to go over the recommended maximum 80-to-90-ish character line width, causing developers working with smaller screens, rotated screens (taller than wide for code), or with multiple windows on a screen I’ve posted a picture below of the 80 character width inserted with comments into my current project below. Notice how close the 80 character limit is to the edge of the screen. If you are wondering, this is the Sublime text editor with SublimeLiner-luacheck and SublimeLinter-gcc installed. I recommend Sublime Text 3! (Although Lua is difficult to install on Windows machines…)
- Looking closer at the constants table, I suspect that specific modules will only use specific sections. I’d recommend breaking up this table in any way you see fit to satisfy the principle of least privilege – code shouldn’t be able to do or to see more than it is supposed to do/see.
- I like how in the var table you initialize some fields to nil. This makes your code act as documentation, which is the best form of documentation. Going down the rabbit hole, comments may become out of date when the code they comment is updated. Same for other forms of documentation. However, when the code highly readable and properly written it is able to act as its own documentation. Variable names and function names are usually true to their use and changed if not. Functions which have at least and at most a single return statement make their return types more apparent. Back to initializing some fields, this actually causes a slight difference in the way the table is created. Instead of
lua_newtable(lua_State *L) being called internally, lua_createtable(lua_State *L, int narr, int nrec) is called internally with the appropriate number of array indices and hash table records preallocated rather than allocated at run time.
- There is a common, although not required, naming convention for event handler callbacks. They usually start with ‘on’ or ‘On’ followed by the name of the event they are for. This makes mentally associating them to their events possible before you even see the connection made. Again, excellent documentation!
- I like this comment,
--bc table has to be set first, because it draws attention to the fact that the following calls are intentionally asynchronous and should pause execution until they are completed. In larger projects, if functions that are asynchronous are not marked then a accidental call to one during, for example, load time would cause valuable time to be wasted in blocked serialized processing rather than parallel. Translated: angry bosses, slow code, and depression. It is a common practice to label any function with asynchronous code as such by putting Async in their name, like Players:GetCharacterAppearanceAsync().
if inputObj.KeyCode == Enum.KeyCode.R then
if var.weps[var.equipped] then
if var.weps[var.equipped].weptype == "Firearm" then
var.weps[var.equipped]:Reload(constants.modules.clientModules.Gui,constants.Main)
end
end
end
- The above code’s logic can be simplified to this, reducing indentation, using less keywords and other characters not necessary to tell what is going on, and is further from the 80 character limit. Where to place the ‘then’ and placing the 'and’s at the end of the previous line or start of the next line is debated. I note that you have probably unconsciously used the idiom of placing the variable on the left-hand side of the comparison. Nice!
if inputObj.KeyCode == Enum.KeyCode.R
and var.weps[var.equipped]
and var.weps[var.equipped].weptype == "Firearm"
then
var.weps[var.equipped]:Reload(constants.modules.clientModules.Gui,constants.Main)
end
- In the same code above,
var.webs[var.equipped] is fairly long and reused three times. I like making another local variable to temporarily hold this. It is debatable if it is more performant (if it is commonly nil, then it is only accessed once and assigning it to a local variable adds overhead), but it is much easier to read! If the wep isn’t normally nil, then this also saves lua from performing a bunch of indexes and hash table lookups.
local wep = var.weps[var.equipped]
if inputObj.KeyCode == Enum.KeyCode.R
and wep
and wep.weptype == "Firearm"
then
wep:Reload(constants.modules.clientModules.Gui,constants.Main)
end
- I also prefer in most cases separating the connection from the handler. Some will argue with that, but I like separating the static code (what isn’t run when it is encountered, like function declarations) from the execution code. Here is a before and after:
constants.services.UIS.InputEnded:connect(function(inputObj)
var.keysDown[inputObj.KeyCode] = nil
if inputObj.KeyCode == Enum.KeyCode.LeftShift then
constants.funcs.shiftUp()
end
end)
-- top of the file
local UserInputService = game:GetService 'UserInputService'
-- with other function declarations
local function onInputEnded(inputObj)
var.keysDown[inputObj.KeyCode] = nil
if inputObj.KeyCode == Enum.KeyCode.LeftShift then
constants.funcs.shiftUp()
end
end
-- at the bottom with the executable code
UserInputService.InputEnded:Connect(onInputEnded)
- Also note how I changed the UserInputService to a local variable at the top of the file using the full service name. This allows me to easily identify what it is. Also, services needed are often very different between files, so it is a best practice to include the required services in each. This also prevents other code from editing the service out from under you accidentally. As a general principle, all code (in functions, modules, or down to statements) should be as decoupled as possible. This is to prevent bugs caused by other sections of code. Tightly coupled code is a nightmare to maintain because a change in one area may affect most of the program through cascading effects. This means that all shared “state” (like the constants table and vars table) is generally a bad idea. Shared constants is a little more redeemable, since it shouldn’t be edited in the first place, but I’d still condemn it.
Personal nitpicking
- I really like using quotes alone instead of parentheses and quotes. It looks a lot cleaner to me. It is more concise and has less symbols clouding the meaning of the function. Granted, to those unfamiliar with it, it can look strange. That is what I did above with the user services local variable. Some will disagree with me (I started a debate on this while I worked at Roblox, but it wasn’t the most popular opinion
)
- I also like using single quotes instead of double ones. They add less to the first-look complexity of the file. Double quotes require hitting the same key as the single quotes, but with shift.
- For tables, I end every line with a
;, including the last line. This makes adding other elements easy and shows that it is different from other types of lists (argument lists, variable lists, expression lists). I do still use commas though when making a single line array.
High-level Concepts
OOP
I don't like OOP as taught in classes. Java is an abomination in my sight. I prefer functional programming -- the time-tested means of programming so influential that it has prevented pure OOP in most Java and other OO code. The vary basis of math, combinatorics, and lambda calculus (the fundamental form of computing equivalent to a Turing machine and made by the professor of the creator of the Turing machine) is the pure function. ANYWAYS, if you are going to use OOP, you have to do it right. Don't do as Roblox did with their instances and pay for now with a difficult to change API. Instead favor what is called in the biz, "Composition Over Inheritance".
That means that instead of a structure like this:
Object
- VisibleMesh
- MovingMesh
- VisibleMovingSolidMesh
- Player
- Cloud
We have this:
Object
- MeshBehavior
- Visible
- Movable
- Solid
- Mesh
- Player
- Cloud
Where we can add MeshBehavior objects to the meshes of a player or cloud. This makes adding classes like a Building possible without a tons of refactoring as in the previous example by giving it a mesh with Visible and Solid behaviors.
You should also note that Lua doesn’t have built in objects. It is what is called an impure functional language – it can create closures with upvalues and functions affect more then their arguments. It is flexible enough though that OOP can be implemented in multiple ways. Two common methods are through closures and metatables. The most common is metatables though. I didn’t get to see your class implementations, but here is a good layout to follow:
local ClassName = {
defaultProperty = false;
}
local privateStaticProperty = true
function ClassName.new()
return setmetatable({
property = 'foo';
}, ClassName)
end
function ClassName:methodName()
return self.property
end
ClassName.__index = ClassName
return ClassName
So, I’ve gotten to this point and realized that there is other things I need to do (like eat). So if you apply some of the suggestions and ask again, I’ll supply more feedback.
