robertjawoods wrote:
The code isn't finished yet, but the basic concept is working, and rather than building on what is, in my opinion, shoddy code, I'd like to get some feedback on the code I already have.
Your intuition didn't decieve you, your code is pretty bad - it's a good thing you asked.
Usually, when you think there is a better way to code something, there is at least one better way, but sometimes it can take hours to find it.
It's a great shortcut to show other people your code, so you can improve a lot quicker, which will make things a lot easier for you (and potentially others) that have to work with your code.
Most of the stuff I learned myself was through showing other people my bad code quickly, so I wouldn't get bad habits.
So first things first, unfortunately I don't have a whole lot of time, so I won't be reviewing the entire code, but it seems like there are some very basic and useful techniques that you don't know yet that will help you a lot in the future.
So let's get started:
Code: Select all
--18 up, 25 across
function CreateQuads(quadArray, tileSize, tileset, rows, cols, tileKey)
local index, xPos, yPos = 1, 0, 0
for _ = 1, rows do
for __ = 1, cols do
quadArray[tileKey[index]] = love.graphics.newQuad(xPos, yPos, tileSize, tileSize, tileset:getWidth(), tileset:getHeight())
xPos = xPos + tileSize
index = index + 1
end
xPos = 0
yPos = yPos + tileSize
end
end
The comment shouldn't be in your code, because you might want to use this function for different sizes.
To be honest, I'm not entirely sure what you want to do with the tileKey table but since it contains only 4 values, and you don't reset the index in your loop, the only reason that your program doesn't crash is that you use 2 for both rows and cols.
(Because quadArray[tileKey[5]] would be quadArray[nil] which is not allowed.)
This means this function is
not reusable! You don't want to have to rewrite this for every new map you make.
Besides, calling this an array is not the correct term. An array usually has sequential indices starting 1 (in Lua).
So if you have something like quadArray["#"] you should rather call it quadTable or something like that.
If we only look at the code of this function, a lot can be done better as well:
Code: Select all
function CreateQuads( tileSize, tileset, rows, cols, tileKey)
local quadTable = {}
for y = 1, rows do
for x = 1, cols do
local index = (y - 1)*cols + x
quadTable[tileKey[index]] = love.graphics.newQuad((x - 1)*tileSize, (y - 1)*tileSize, tileSize, tileSize, tileset:getWidth(), tileset:getHeight())
end
end
return quadTable
end
You don't need your local variables, because you can get them from the iterators.
What you do in your code is the following:
Code: Select all
TileQuads = {}
CreateQuads(TileQuads, tileSize, tileset, rows, cols, tileKey)
This is bad style because you need to pass an empty table into the function to mutate it, instead of creating an empty table in the function directly and returning it.
Because of the "return quadTable" I put into the function you can now just do this:
Code: Select all
TileQuads = CreateQuads(tileSize, tileset, rows, cols, tileKey)
Phew, this is already taking longer than I have time, so I'll be less detailed now.
This code in SaveMap:
Code: Select all
if tilemap[i][j] ~= nil then
io.write(tilemap[i][j])
else
io.write(".")
end
could be simplified to:
Code: Select all
io.write(tilemap[i][j] or ".") -- or uses the first value if it is not false/nil otherwise it uses the second
In your update function you could use this for your mouse position:
Code: Select all
function clamp( val, min, max )
return math.max( min, math.min( max, val ) )
end
function love.update( dt )
xPos, yPos = clamp(love.mouse.getX(), 0, maxX), clamp(love.mouse.getY(), 0, maxY) -- define maxX and maxY in love.load because it never changes, does it?
-- don't set the window title in update, but in the SaveMap function
end
About the way you handle input, it's
ok for small projects, but I wrote a
blog post about it that you're interested.
That's all for now! Hope it helps!