Code review?

[robertjawoods]

Hi guys, can anyone recommend any sites where I could get my code reviewed? My code works as expected but as I taught myself to code rather than through a Comp Sci course, I feel that my code is awfully written and that I'd benefit from someone having a look over my code and telling me how I could do things better.

Thanks in advance.

[ivan]

If you post a short script or .love file on here I'll take a look.
Note that there are different types of Lua code based on the programmer's motivation.
-optimized code is usually ugly and long
-short code is easy to maintain but hard to understand
-descriptive code (like the one you see in tutorials) is easy to read and understand but not optimal
In my opinion it's a balance between these three factors.

[robertjawoods]

Hi there, thanks for your reply. I can see that there are distinctions in the way you right code (better design vs speed of development etc.), but personally I don't think I know enough for that to come into it yet.

The code in question is a Tile Map editor for a tutorial on tiles that I followed. Here's the code:

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

function AddTile(tilemap, mouseX, mouseY, tilekey)
    local yTile, xTile = math.floor(mouseY / 32)  + 1, math.floor(mouseX / 32) + 1
    tilemap[yTile][xTile] = tilekey
end

function RemoveTile(tilemap, mouseX, mouseY)
    local yTile, xTile = math.floor(mouseY / 32)  + 1, math.floor(mouseX / 32) + 1
    tilemap[yTile][xTile] = nil
end

function SaveMap(path, tilemap)
  local map = io.open("C:\\Users\\Robert\\Desktop\\map.txt", "w")
  io.output(map)
  for i = 1, 18 do
    for j = 1, 25 do
      if tilemap[i][j] ~= nil then
        io.write(tilemap[i][j])
      else
        io.write(".")
      end
    end
    if i < 18 then
      io.write("\n")
    end
  end
  io.close(map)
  saved = true
end

function love.load(args)
    --Zerobrane debugging
    if arg[#arg] == "-debug" then
          require("mobdebug").start()
    end

    -- Is the map saved? On opening map will not have been saved
    saved = false

    -- Hide the mouse cursor
    love.mouse.setVisible(false)

      -- Loads the tileset
    tileset = love.graphics.newImage('countryside.png')
    x, y, tileSize = nil, nil, 32

    -- Index of the current tile to be drawn, corresponds with tile key
    currentTile = 1

    -- Table to hold string representation of tiles
    TileMap = {}

    rows, cols = 2, 2

    -- Table with tile keys to allow incrementing of tiles
    tileKey = {' ', '#', '*', '^'}

    -- Table of tile quads
    TileQuads = {}
    CreateQuads(TileQuads, tileSize, tileset, rows, cols, tileKey)

    -- Iterate through array, create another table for each y, for a 2D array
    for i = 1,18 do
        TileMap[i] = {}
    end
end

function love.update(dt)
  -- Ensures the edges of the tiles never pass the edge of the screen
    if love.mouse.getX() <= 0 then
        xPos = 0
    elseif love.mouse.getX() >= love.graphics.getWidth() - tileSize then
        xPos = love.graphics.getWidth() - 32
    elseif love.mouse.getY() <= 0 then
        yPos = 0
    elseif love.mouse.getY() >= love.graphics.getHeight() - tileSize then
        yPos = love.graphics.getHeight() - tileSize
    else
        xPos, yPos = love.mouse.getX(), love.mouse.getY()
    end

    -- Checks to see if the tilemap is saved, changing the title if not saved
    if not saved then
      love.window.setTitle("Tile Map Editor - *")
    else
      love.window.setTitle("Tile Map Editor")
    end

end

function love.keypressed(key, isrepeat)
    -- Change tiles, wraps around if end is reached.
    if key == "right" then
      if currentTile < rows * cols then
          currentTile = currentTile + 1
      else
          currentTile = 1
      end
    elseif key == "left" then
      if currentTile > 1 then
          currentTile = currentTile - 1
      else
          currentTile = rows * cols
      end
    elseif key == "s" then
      SaveMap("C:\\Users\\Robert\\Desktop\\map.txt", TileMap)
    elseif key == 'escape' then
      love.event.quit()
    end
end

function love.mousepressed(x, y, button, istouch)
  saved = false
   if button == 1 then -- the primary button
      AddTile(TileMap, x, y, tileKey[currentTile])
    elseif button == 2 then
      RemoveTile(TileMap, x, y)
    end
end

function love.draw()
    -- Draws tile map, black if nil
    for yAxis = 1, 18 do
        for xAxis = 1, 25 do
            if TileMap[yAxis][xAxis] ~= nil then
                local index = TileMap[yAxis][xAxis]
                love.graphics.draw(tileset, TileQuads[index], (xAxis - 1) * 32, (yAxis - 1) * 32)
            end
        end
    end

    love.graphics.draw(tileset, TileQuads[tileKey[currentTile]], xPos, yPos)
end

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.

[ivan]

Hi there. The code is ok, it's a good starting point.

First I have to point out that "quads" are rectangles on your spritesheet image.
Much easier to index these by "name" or "id" rather than keep them in an 2d array!
Something like:

Code: Select all

function defineQuad(image, col, row, ncols, nrows)
  local w, h = image:getWidth(), image:getHeight()
  local tw, th = w/ncols, h/nrows
  local x, y = tw*col, th*row
  return love.graphics.newQuad(x, y, tw, th, w, h)
end
-- let's keep our quads indexed by name:
quads = {}
-- the first sprite on the spritesheet (0,0) is grass:
quads['grasstile'] = defineQuad(image, 0, 0, 10, 10)

That's how I would handle quads, much easier to reuse these.
Next you have the map "array".
Don't write functions like "AddTile(tilemap, mouseX, mouseY, tilekey)"
This is your map code, and your map code shouldn't know about the mouse.
A more appropriate function would be "AddTile(tilemap, tileX, tileY, quadID)"

I realize that my points seem like nitpicking but in general these are the 2 primary things
that I would be concerned about:
1. understanding how love2d objects work
2. keeping the code domain specific (map, input, game logic, rendering code)
3. keeping your functions as narrow in scope as possible so you can reuse them later

Yea, it's a good start so keep at it!

[undef]

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!

[robertjawoods]

Thanks for your input guys, I'm going to implement the changes you mentioned right away

[adge]

Could you maybe share the link to the tutorial? I'm looking for something like that too!