The "cleanliness" of code is an interesting thing... I think it's important to think of what kinds of things make code "clean", and what the actual practical benefits are. (I used to think that splitting programs up into lots of different files and having classes for everything and having lots of different functions meant having "clean code", but I don't think I was thinking about what it fundamentally meant for code to be "clean" and what the practical benefits were, I was just doing stuff because "everyone" said it was good.)
Looking at the code you have, here are some "cleanliness" ideas. These are just what I believe at the moment, I'm not a good programmer and I am no authority on this stuff.
Number 1: Consistency
What benefit it has: When things are consistent, they're easier to remember because they're like all of the other things. (Plus there's some kind of aesthetic feeling of cleanliness with it.)
One place I thought that consistency could make this code "cleaner" is where some names are use underscores (specifically player_x, player_y and bg_image) and some use camelCase.
So, the first thing I did was made everything camelCase.
Number 2: Non-duplication
What benefit it has: When code or a value exists in only one place, it can be changed in one place instead of many, which can be quicker to do and maybe more reliable because there aren't places to change that you could miss. It also makes the code smaller, which can make it easier to navigate, and, perhaps the main reason, easier to think about.
Looking at the code, I can see two places where the code is very similar: creating the quads, and moving the player.
Looking at this code:
Code: Select all
walkingFramesDown[1] = love.graphics.newQuad(0, 0, 29, 57, player:getDimensions())
walkingFramesDown[2] = love.graphics.newQuad(29, 0, 29, 57, player:getDimensions())
walkingFramesDown[3] = love.graphics.newQuad(58, 0, 29, 57, player:getDimensions())
walkingFramesDown[4] = love.graphics.newQuad(87, 0, 29, 57, player:getDimensions())
walkingFramesUp[1] = love.graphics.newQuad(0, 171, 29, 57, player:getDimensions())
walkingFramesUp[2] = love.graphics.newQuad(29, 171, 29, 57, player:getDimensions())
walkingFramesUp[3] = love.graphics.newQuad(58, 171, 29, 57, player:getDimensions())
walkingFramesUp[4] = love.graphics.newQuad(87, 171, 29, 57, player:getDimensions())
walkingFramesRight[1] = love.graphics.newQuad(0, 114, 29, 57, player:getDimensions())
walkingFramesRight[2] = love.graphics.newQuad(29, 114, 29, 57, player:getDimensions())
walkingFramesRight[3] = love.graphics.newQuad(58, 114, 29, 57, player:getDimensions())
walkingFramesRight[4] = love.graphics.newQuad(87, 114, 29, 57, player:getDimensions())
walkingFramesLeft[1] = love.graphics.newQuad(0, 57, 29, 57, player:getDimensions())
walkingFramesLeft[2] = love.graphics.newQuad(29, 57, 29, 57, player:getDimensions())
walkingFramesLeft[3] = love.graphics.newQuad(58, 57, 29, 57, player:getDimensions())
walkingFramesLeft[4] = love.graphics.newQuad(87, 57, 29, 57, player:getDimensions())
it can be seen that the differences between the lines are the table to store the quad, the table index, and the X and Y position for the quad.
What changes to make to reduce the duplication will depend on things like how you think the code will change over time, and I'm not very good at this stuff, but here's just some ideas.
My first thought was "just create a function which takes the things which change between the lines".
Code: Select all
local function newFrame(frameTable, frameIndex, x, y)
frameTable[frameIndex] = love.graphics.newQuad(0, 0, x, y, player:getDimensions())
end
newFrame(walkingFramesDown, 1, 0, 0)
newFrame(walkingFramesDown, 2, 29, 0)
newFrame(walkingFramesDown, 3, 58, 0)
newFrame(walkingFramesDown, 4, 87, 0)
newFrame(walkingFramesUp, 1, 0, 171)
newFrame(walkingFramesUp, 2, 29, 171)
newFrame(walkingFramesUp, 3, 58, 171)
newFrame(walkingFramesUp, 4, 87, 171)
newFrame(walkingFramesRight, 1, 0, 114)
newFrame(walkingFramesRight, 2, 29, 114)
newFrame(walkingFramesRight, 3, 58, 114)
newFrame(walkingFramesRight, 4, 87, 114)
newFrame(walkingFramesLeft, 1, 0, 57)
newFrame(walkingFramesLeft, 2, 29, 57)
newFrame(walkingFramesLeft, 3, 58, 57)
newFrame(walkingFramesLeft, 4, 87, 57)
These lines are still pretty similar though, with the 1 to 4 sequence, so I guess the only difference between the sections is the table and the Y offset.
Code: Select all
local function newFrames(frameTable, y)
for frameIndex = 1, 4 do
frameTable[frameIndex] = love.graphics.newQuad((frameIndex - 1) * 29, y, 29, 57, player:getDimensions())
end
end
newFrames(walkingFramesDown, 0)
newFrames(walkingFramesUp, 171)
newFrames(walkingFramesRight, 114)
newFrames(walkingFramesLeft, 57)
So that went from 19 lines to 10. Maybe this isn't suitable for your game though, or maybe you'd prefer doing things a different way, and I'm not saying that what I did was "good", but the basic idea is that you can remove duplication and it can have benefits.
With the player movement code:
Code: Select all
if love.keyboard.isDown("down") then
isWalking = true -- I just have this in case I need to use it later
playerY = playerY + playerSpeed * dt
if (elapsedTime > 0.1) then
if (currentFrame < 4) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesDown[currentFrame]
elapsedTime = 0
end
end
if love.keyboard.isDown("up") then
isWalking = true
playerY = playerY - playerSpeed * dt
if (elapsedTime > 0.1) then
if (currentFrame < 4) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesUp[currentFrame]
elapsedTime = 0
end
end
if love.keyboard.isDown("right") then
isWalking = true
playerX = playerX + playerSpeed * dt
if (elapsedTime > 0.1) then
if (currentFrame < 4) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesRight[currentFrame]
elapsedTime = 0
end
end
if love.keyboard.isDown("left") then
isWalking = true
playerX = playerX - playerSpeed * dt
if (elapsedTime > 0.1) then
if (currentFrame < 4) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesLeft[currentFrame]
elapsedTime = 0
end
end
I can see that the only difference between these parts is the key which is down and the direction the player is moving (which is based on the key).
So, maybe you could do something like this (except you'd probably want a better name than "movePlayer" because that sounds like it moves the player, but whatever).
Code: Select all
local function movePlayer(key)
if love.keyboard.isDown(key) then
isWalking = true
if key == "down" then
playerY = playerY + playerSpeed * dt
elseif key == "up" then
playerY = playerY - playerSpeed * dt
elseif key == "right" then
playerX = playerX + playerSpeed * dt
elseif key == "left" then
playerX = playerX - playerSpeed * dt
end
if (elapsedTime > 0.1) then
if (currentFrame < 4) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesUp[currentFrame]
elapsedTime = 0
end
end
end
movePlayer("down")
movePlayer("up")
movePlayer("right")
movePlayer("left")
57 lines to 30, I think. See how it's kind of easier to see what's going when stuff isn't repeated, because there's less to read?
I just noticed the number of frames per animation (4) is repeated as well, so you can make that into a variable.
Code: Select all
walkingFrameCount = 4
local function newFrames(frameTable, y)
for frameIndex = 1, 4 do
-- etc.
local movePlayer(key)
-- etc.
if (currentFrame < walkingFrameCount) then
What benefit it has: If you're reading the code which uses the variable/function, it's nearby to read if you need to. And if read a variable/function somewhere where it's not used, you might be unsure of where it actually is used. I dunno.
For example, the player speed is only used when moving the player, so you can make it near the code which uses it.
Code: Select all
local function movePlayer(key)
if love.keyboard.isDown(key) then
isWalking = true
local playerSpeed = 200
if key == "down" then
playerY = playerY + playerSpeed * dt
elseif key == "up" then
playerY = playerY - playerSpeed * dt
elseif key == "right" then
playerX = playerX + playerSpeed * dt
elseif key == "left" then
playerX = playerX - playerSpeed * dt
end
if (elapsedTime > 0.1) then
if (currentFrame < walkingFrameCount) then
currentFrame = currentFrame + 1
else
currentFrame = 1
end
activeFrame = walkingFramesUp[currentFrame]
elapsedTime = 0
end
end
end
What benefit it has: It's hard to know what you'll actually need in the future, so you might spend time doing things you don't actually need.
For example, the variables like isWalking and directionRight can be saved until you have a reason to use them.
Most importantly though,
none of this stuff really matters. I remember briefly looking at the source code for
Mari0 and thinking to myself "this doesn't look particularly clean", but it doesn't matter, it could have the cleanest code in the world or the uncleanest code in the world, but it got made and it's one of the best games made with LOVE.
And yeah generally people do use the support section for these types of questions, you might get more responses there.
