[Duta]

So I just wrote my first program in Lua and was wondering if anyone with knowledge of the language check if over for good programming style; as I'm new to the language I don't know if my way is considered 'correct'.

--Character class
Character = {}
Character.__index = Character
function Character:new()
	local self = {}
	setmetatable(self, Character)
	self.name  = "Unknown"
	self.level = "Unknown"
	self.class = "Unknown"
	self.race  = "Unknown"
	return self
end
function Character:load(name, level, class, race)
	if name  ~= nil then self.name  = name  end
	if level ~= nil then self.level = level end
	if class ~= nil then self.class = class end
	if race  ~= nil then self.race  = race  end
	return self
end

--Util functions
Util = {}
function Util:concat(aString, length)
	local blanks = string.rep(" ", length)
	local s = aString..blanks
	return string.sub(s, 0, length)
end
function Util:printtable(items, vars)
	local div = "|"
	for i, var in ipairs(vars) do
		local maxlength = 0
		for i, item in ipairs(items) do
			local len = string.len(item[var.varname])
			if len > maxlength then
				maxlength = len
			end
		end
		div = div..string.rep("-", maxlength).."|"
		var.maxlength = maxlength
	end
	print(div)
	local totlen = 0
	local numvars = 0
	io.write("|")
	for i, var in ipairs(vars) do
		io.write(Util:concat(var.name, var.maxlength).."|")
		totlen = totlen + var.maxlength
		numvars = numvars + 1
	end
	print()
	print(div)
	for i, item in ipairs(items) do
		io.write("|")
		for i, var in ipairs(vars) do
			io.write(Util:concat(item[var.varname], var.maxlength).."|")
		end
		print()
	end
	print(div)
end

--Var class
Var = {}
Var.__index = Var
function Var:new()
	local self = {}
	setmetatable(self, Var)
	return self;
end
function Var:load(varname, name)
	self.varname = varname
	self.name = name
	return self
end

--Test chars
local chars = {
	Character:new():load("Painrc", 25,  "Rogue", "Orc"  ),
	Character:new():load("Geoff",  90,  "Mage",  "Gnome"),
	Character:new():load("Anon",   nil, "/b/",   "4chan")
}

--Print test chars
Util:printtable(chars, {
	Var:new():load("name",  "Name" ),
	Var:new():load("level", "Level"),
	Var:new():load("class", "Class"),
	Var:new():load("race",  "Race" )
})
for i,char in ipairs(chars) do
	print(char.name, char.isHorde())
end

This post has been edited by Duta: 29 October 2012 - 10:43 AM

[ishkabible]

--Character class
Character = {}
Character.__index = Character
function Character:new()
	local self = {}
	setmetatable(self, Character)
	self.name  = "Unknown"
	self.level = "Unknown"
	self.class = "Unknown"
	self.race  = "Unknown"
	return self
end

'nil' is generally a better value for saying that the value is unknown, that's what it's there for.

function Character:load(name, level, class, race)
	if name  ~= nil then self.name  = name  end
	if level ~= nil then self.level = level end
	if class ~= nil then self.class = class end
	if race  ~= nil then self.race  = race  end
	return self
end

the 'if' statements have no effect if you us 'nil' for the unknown value in the first place; hence 'nil' works out a bit better.

function Character:isHorde()

   if ( self.race == "Human"        or
        self.race == "Night Elf"    or
        self.race == "Dwarf"        or
        self.race == "Gnome"        or
        self.race == "Draenei"
      )
   then return false;
   else return true;
   end
end

this is ok, I might use a 'is element of function' with a table containing all those races but that's just me.

--Util functions
Util = {}
function Util:concat(aString, length)
	local blanks = string.rep(" ", length)
	local s = aString..blanks
	return string.sub(s, 0, length)
end

I don't see where 'self' is used here yet you declare it ':' not '.'. also you can use __concat function in your objects metatable so that the '..' operator can be used.

function Util:printtable(items, vars)
	local div = "|"
	for i, var in ipairs(vars) do
		local maxlength = 0
		for i, item in ipairs(items) do
			local len = string.len(item[var.varname])
			if len > maxlength then
				maxlength = len
			end
		end
		div = div..string.rep("-", maxlength).."|"
		var.maxlength = maxlength
	end
	print(div)
	local totlen = 0
	local numvars = 0
	io.write("|")
	for i, var in ipairs(vars) do
		io.write(Util:concat(var.name, var.maxlength).."|")
		totlen = totlen + var.maxlength
		numvars = numvars + 1
	end
	print()
	print(div)
	for i, item in ipairs(items) do
		io.write("|")
		for i, var in ipairs(vars) do
			io.write(Util:concat(item[var.varname], var.maxlength).."|")
		end
		print()
	end
	print(div)
end

Again, I don't see where 'self' is used here. and all those 'concat' calls could be just a simple '..' operator

--Var class
Var = {}
Var.__index = Var
function Var:new()
	local self = {}
	setmetatable(self, Var)
	return self;
end

this is fine

function Var:load(varname, name)
	self.varname = varname
	self.name = name
	return self
end

fine I suppose

--Test chars
local chars = {
	Character:new():load("Painrc", 25,  "Rogue", "Orc"  ),
	Character:new():load("Geoff",  90,  "Mage",  "Gnome"),
	Character:new():load("Anon",   nil, "/b/",   "4chan")
}

--Print test chars
Util:printtable(chars, {
	Var:new():load("name",  "Name" ),
	Var:new():load("level", "Level"),
	Var:new():load("class", "Class"),
	Var:new():load("race",  "Race" )
})

rather than Var:new():load("race", "Race" ) by adding those arguments to your 'new' constructor you could just make it Var.new("race", "Race");. same with Character. even cooler you could overload the call operator for 'Var' or 'Character' so that it was just Var("race", "Race"); or if you were feeling fancy check for a table and use it

Var{"race", "Race"}

spark notes:

• with some clever use of operator overloading and nil you can reduce your code size by a good bit
  
• use '.' if a table just acts as a namespace or static function ':' if it's used with an object. ':' passes an unused argument otherwise
  
• you are introducing unnecessary calls, namely with 'load'

This post has been edited by ishkabible: 29 October 2012 - 11:05 AM

[Duta]

Thanks ishka, that was really useful
I tried changing Util:concat and Util:printtable to Util.concat and Util.printtable (in the function declarations and the uses), however doing that gives me the output:

||||
||||
||||
||||

Whereas normally I get:

|------|-------|-----|-----|
|Name  |Level  |Class|Race |
|------|-------|-----|-----|
|Painrc|25     |Rogue|Orc  |
|Geoff |90     |Mage |Gnome|
|Anon  |Unknown|/b/  |4chan|
|------|-------|-----|-----|

Any ideas?
Also, I didn't mean to include the Character:isHorde function as I don't use it, but good point. I wasn't happy with it as it stood
Finally, how would you overload the call operator?

[ishkabible]

did you change it every where? if you change the definition but not the calls it will pass an extra argument.

if you define a function with ':' it adds an implicit self parameter

function x:y() end

is the same as

function x.y(self) end

which is also the same as

x.y = function(self) end

which is short hand further for

x["y"] = function(self) end

when you call a function like so

x.y()

it's the same as

(x["y"])()

or

f = x["y"]
f()

when you use ':' however it's different.

x:y(1, 2, 3)

is the same as...

x.y(x, 1, 2, 3)

which again is the same as

(x["y"])(x, 1, 2, 3)