Code review please

[knuxyl]

I just switched from pygame due to lack of hardware acceleration and I'm not sure the best way of coding in lua. If there is anything I should do differently in the code below please let me know. It is just a simple sprite sheet loader that'll display a quad based on what frame number you give. I made it as simple as possible. Before I get too far into porting what I've done with pygame I want to make sure I'm doing this the most efficient and easy to read way. Thanks

Code: Select all

-------------------------------------------------------------
player = {}
player.q = {}
player.spritesheet = {}
scale = 4
frame = 1
love.graphics.setDefaultFilter("nearest")
-------------------------------------------------------------
function player:create(f, w, h)
  self.w = w
  self.h = h
  self.spritesheet.f = f
  self.spritesheet.i = love.graphics.newImage(self.spritesheet.f)
  self.spritesheet.w, self.spritesheet.h = self.spritesheet.i:getDimensions()
  self.spritesheet.c = self.spritesheet.w / self.w
  self.spritesheet.r = self.spritesheet.h / self.h
  count = 1
  for r = 0, self.spritesheet.r - 1 do
    for c = 0, self.spritesheet.c - 1 do
      self.q[count] = love.graphics.newQuad(c * self.w, r * self.h, self.w, self.h, self.spritesheet.w, self.spritesheet.h)
      count = count + 1
    end
  end
end
-------------------------------------------------------------
function love.load()
  player:create("assets/sprites/rocket.png", 32, 32)
end
-------------------------------------------------------------
function love.update(dt)

end
-------------------------------------------------------------
function love.draw()
  love.graphics.setBackgroundColor(131/255, 192/255, 240/255, 1)
  love.graphics.draw(player.spritesheet.i, player.q[frame], 0, 0, 0, scale)
end
-------------------------------------------------------------

[raidho36]

First of all, you defined all of your variables as global, which you probably shouldn't, try using local variables whereever possible (keep in mind the hard-coded limit of 200 locals in any given scope).

Second of all, initialization functions should go into the load callback (your filtering setup function and the like).

Third, you set up entities as basic tables instead of using classes (technically, metatables). You can set up metatables manually as well as use any of the myriad of class libraries, I of course recommend my own.

Lastly, you use 8 bit integer color values, LOVE uses floating point color.

[TheHUG]

You have some one-letter variable names where it isn't immediately obvious what they are.
(Q, I ,f w, c), I think they'd be better off as full words.

Most coding standards agree that it is best to limit the number of characters per line to around 80 characters.

[knuxyl]

First of all, you defined all of your variables as global, which you probably shouldn't, try using local variables whereever possible (keep in mind the hard-coded limit of 200 locals in any given scope).

Second of all, initialization functions should go into the load callback (your filtering setup function and the like).

Third, you set up entities as basic tables instead of using classes (technically, metatables). You can set up metatables manually as well as use any of the myriad of class libraries, I of course recommend my own.

Lastly, you use 8 bit integer color values, LOVE uses floating point color.

well player is really the only global, while everything else is a variable in player. I don't know which variables ill need later on so i think this should be ok. is it better on cpu to use locals when possible? i could probably localize player.c and player.r but i might need everything else. if someone could explain the importance of localized variables in my code it would help me wrap my head around it.

by the load cappback do you mean the love.load()?

and player doesnt really need to be a class since there will only be one right? havent heard of metatables, ill look into it.

and about the one letter variables, my code gets hard to read with long variable names and i always use the same letter for what its referencing, ie; i = image, w= width, c = columns, q = quad, etc

[bobbyjones]

The importance of using only locals is to mainly prevent name conflicts. This can introduce some pretty crazy bugs. For example, if you declare player in two files and each has conflicting data and or operations done to them it would cause the player to act unpredictably. You can't localize player.c or player.r. Only the table containing those variables. You have to design your code to only declare player in one location and pass it to any function that needs it. For example:

Code: Select all

	--using globals
	player = {}
	player.x = 1
	player.y = 1
	
	function movePlayer() --this will always affect the player in the current scope. It can cause problems if this function is called in another file and you forget what exactly it does.
		player.x = 100
		player.y = 100
	end
	
	--using locals
	local player = {}
	player.x = 1
	player.y = 1
	
	function movePlayer(player) --You can also make this function a generic moveObject or moveEntity function since it will be operating on a passed in table
		player.x = 100
		player.y = 100
		return player --tables are passed by reference you do not have to return the table at all. This is effectively useless. But you can make the function create a new table to overwrite the old one.
	end

[knuxyl]

Ok, I wasn't sure if global or local took longer to load or something. What I'm creating is rather simple so having global variables shouldn't be an issue. Thanks!

[TheHUG]

Ok, I wasn't sure if global or local took longer to load or something. What I'm creating is rather simple so having global variables shouldn't be an issue. Thanks!

In this case, count, for example, is a global variable. In lua, unlike python, variable scope is global unless explicitly declared to be local. Globals can very easily get out of hand in lua because of this, so I'd really suggest getting into the habit of declaring all variables local unless you specifically need them to be global.

[pgimeno]

In lua, unlike python, variable scope is global unless explicitly declared to be local.

Furthermore, in Python, globals are not actually global, but module-local; however, in Lua, they are really global, i.e. application-wide, and therefore in the same namespace for all modules.

Globals are a bit slower than locals, because they usually need to be looked up in a table called the environment (by default you can access the environment using _G), while locals are "hardcoded" into the bytecode.

[knuxyl]

In lua, unlike python, variable scope is global unless explicitly declared to be local.

Furthermore, in Python, globals are not actually global, but module-local; however, in Lua, they are really global, i.e. application-wide, and therefore in the same namespace for all modules.

Globals are a bit slower than locals, because they usually need to be looked up in a table called the environment (by default you can access the environment using _G), while locals are "hardcoded" into the bytecode.

That is exactly what I was wondering, if it had to be looked up or hard coded.