How's my style?

[waraiotoko]

I've been programming a SRPG for a few days now and am slowly building useful tables and functions to use during development.
I really don't have someone who can comment on my style in person, so I figured that some advice from the Love2D community would be nice.
This code stands on it's own, so you can throw it into a main.lua function if you need to test it. I'm not posting everything because most of it is too specific to my game to stand on it's own. I just want some comments, I can post more code if you want, but I'm not sure how easy the rest of it is to read.

Thanks!

Here's a snippet from my ui.lua file, lines are broken into a table so that I can create simple menus in the future. It's in early-development.

Code: Select all

ui = {}
ui.windowstyles = {}
ui.windowstyles.default = 
{
	bgcolor = {0,0,0},
	textcolor = {255,255,255},
	textpad = 5,
	font
}

function ui:load()
	ui.windowstyles.default.font = love.graphics.newFont("fonts/AlegreyaSC-Regular.ttf")
end

function ui:drawTextWindow(texttable, x, y, stylekey)
	local style = self.windowstyles[stylekey];
	love.graphics.setFont(style.font)
	--find max line length and height
	local width = 0;
	local height = 0;
	for key,text in pairs(texttable) do
		if style.font:getWidth(text) > width then
			width = style.font:getWidth(text)
		end
		height = height+1;
	end
	height = style.font:getHeight()*height;
	--draw the background
	love.graphics.setColor(unpack(style.bgcolor))
	love.graphics.rectangle("fill", x, y, width+(2*style.textpad), height+(2*style.textpad))
	--draw the text
	love.graphics.setColor(unpack(style.textcolor))
	for key,text in pairs(texttable) do
		love.graphics.print(text, x+style.textpad, y+(style.font:getHeight()*(key-1))+style.textpad)
	end
end

And here's an example call:

Code: Select all

self:drawTextWindow(
			{"Name: David", 
			"Level: 1", 
			"Class: Warrior"},
			300, 300, "default")

[kikito]

It looks very good. I have three comments.

First one: semicolons aren't really needed. You seem to know this, because you haven't used them in all the lines. My advice is getting rid of all of them. In Lua, they don't have any purpose. Less typing, and your code will look a little more clear.

The second comment is on indentation of tables in calls; If I had had exactly that second function call, I would have made it a oneliner:

Code: Select all

self:drawTextWindow( {"Name: David", "Level: 1", "Class: Warrior"}, 300, 300, "default")

Had the text been longer, I would have divided the statement in two:

Code: Select all

local windowText = {
  "Name: David and a lot more text here",
  "Level: 1 and more text here ",
  "Class: Warrior Prince of the Darkness and Very Strong"
}
self:drawTextWindow(windowText, 300, 300, "default")

The last comment is that ui:drawTextWindow's implementation is just too big. It does too many things. You seem to understand that; you have tried to "make it easier to understand" by adding comments. Instead, move each chunk to its own function (probably a local private function), and give that a proper name:

Code: Select all

local function calculateTextTableDimensions(texttable, font)
   local width, nLines = 0, #texttable
   for i=1,nLines do -- numeric for
      width = math.max(width, font:getWidth(texttable[i])) -- removed one if using math.max here
   end
   local height = font:getHeight() * nlines
   return width, height
end

local function drawTextWindowBackground(x,y,texttable, style)
   local width, height = calculateTextTableDimensions(texttable, style.font)
   love.graphics.setColor(style.bgcolor)
   love.graphics.rectangle("fill", x, y, width + 2 * style.textpad, height + 2 * style.textpad)
end

local function drawTextWindowText(x,y,texttable,style)
   love.graphics.setColor(style.textcolor)
   local font, textpad = style.font, style.textpad
   love.graphics.setFont(font)
   for i=1,#texttable do
      love.graphics.print(text[i], textpad + x, textpad + y + font:getHeight() * (i-1))
   end
end

function ui:drawTextWindow(texttable, x, y, stylekey)
   local style = self.windowstyles[stylekey]
   drawTextWindowBackground( x, y, texttable, style)
   drawTextWindowText(x, y, texttable, style)
end

Minor changes in the code:

• Calculation of the box height can be simplified using # to obtain the number of lines of texttable.
• You can avoid that if in the calculation of the width by using math.max. It is also more explicit - your code is saying "hey, I'm calculating a max" instead of "hey, I'm doing something if something happens".
• Since textable is an array-like table, you can parse it using a numeric array. This is faster than using pairs.
• love.graphics.setColor admits a table, so you don't need to call unpack on the colors
• I've used two local variables called font and textpad on the drawTextWindowText. Note how that makes the last line clearer, since the code is shorter (it also has the side effect of making it go a bit faster).
• I have removed unnecessary parenthesis: a + (b * c) is the same as a + b * c. The multiplication has higher priority, and gets done before the addition.

Regards!

[waraiotoko]

The last comment is that ui:drawTextWindow's implementation is just too big. It does too many things. You seem to understand that; you have tried to "make it easier to understand" by adding comments. Instead, move each chunk to its own function (probably a local private function)...

Thanks much for the comments! I'll be sure to look over them all later, but one thing caught my eye: "local private function".
In your implementation, you have local functions, which aren't "members" of the table "ui". Is this what you're referring to when you say "local private function"? I'm used to more OOP languages, and I'm not really willing to use a separate OOP library to add classes to Lua, so I'm still struggling with the proper use of (plain) Lua in terms of OOP.

Right now I treat each file as a "class", and try to have no more than one global table per file. It looks like that's the way you're approaching it.

[coffee]

I wish have a "style" of code" like yours! Mine is awful.

[*]I have removed unnecessary parenthesis: a + (b * c) is the same as a + b * c. The multiplication has higher priority, and gets done before the addition.[/list]

That's so true kikito, but for readibility I also tend to do the same "mistake". Humans!

[kikito]

Thanks much for the comments! I'll be sure to look over them all later, but one thing caught my eye: "local private function".
In your implementation, you have local functions, which aren't "members" of the table "ui". Is this what you're referring to when you say "local private function"?

Yes. A function defined like this:

Code: Select all

local function foo(...)
  ...
end

Will only be visible inside the file it is defined in.

I'm used to more OOP languages, and I'm not really willing to use a separate OOP library to add classes to Lua, so I'm still struggling with the proper use of (plain) Lua in terms of OOP.

Lua has its own thing; most of the time you don't need full OOP to work with Lua. It takes a while to get used to that (I'm still strugglin)

Right now I treat each file as a "class", and try to have no more than one global table per file. It looks like that's the way you're approaching it.

You actually can have ZERO global variables in a file. You define all of them local, and return the "important one" on the last line:

Code: Select all

-- Window.lua: defines a window
local window = {} -- this is the important object

-- create lots of private stuff here; add methods to window like this:

function window:foo()
...
end

-- at the end, return the important object; zero global variables
return window

When you return a value from a file, it gets returned form the require:

Code: Select all

-- main.lua
local window = require 'Window'

If you require the same file more than once in different places, it "gets cached"; if several files do require 'window', they all share the same table, and the code of Window.lua is evaluated only once.