1
\$\begingroup\$

I'm starting to learn Lua and in one part of the book I need to convert a "Maze Game" that uses goto to another one that doesn't use goto.

The maze is the simplest ever made, starting from room #1:

---------
| 1 | 2 |
---------
| 3 | 4 |
---------

This is the original version using goto from the book:

goto room1 -- initial room
::room1:: do
  local move = io.read()
  if move == "south" then goto room3
  elseif move == "east" then goto room2
  else
    print("invalid move")
    goto room1 -- stay in the same room
  end
end

::room2:: do
  local move = io.read()
  if move == "south" then goto room4
  elseif move == "west" then goto room1
  else
    print("invalid move")
    goto room2
  end
end

::room3:: do
  local move = io.read()
  if move == "north" then goto room1
  elseif move == "east" then goto room4
  else
    print("invalid move")
    goto room3
  end
end

::room4:: do
  print("Congratulations, you won!")
end

And this is my attempt:

local rooms = {}
rooms[1] = {
  south=3,
  east=2
}
rooms[2] = {
  south=4,
  west=1
}
rooms[3] = {
  north=1,
  east=4
}
currentRoom = 1
repeat
  local move = io.read()
  local room = rooms[currentRoom][move]
  if room == nil then
    room = currentRoom
    print("Invalid move")
  elseif room == 4 then
    print("Congratulations, you won!")
  end
  currentRoom = room or currentRoom
until currentRoom == 4 

The above code is working but I was wondering if there is something I need to change to improve it somehow.

\$\endgroup\$
  • \$\begingroup\$ Have you tested your attempt? Does your attempt work? \$\endgroup\$ – Simon Forsberg Mar 22 '16 at 17:09
  • \$\begingroup\$ @SimonForsberg hi, yest it does work. I'll update my question \$\endgroup\$ – César Mar 22 '16 at 17:10
3
\$\begingroup\$

I think overall is good, some bits can be improved but it depends on taste. Take the following comments as suggestions.

As rooms is an array, can be defined in a shorter way as:

local rooms = {
   { south=3, east=2 },
   { south=4, west=1 },
   { north=1, east=4 },
}

Variable currentRoom can be localized:

local currentRoom = 1

In Lua can compare a var to nil or ask not var:

if not room then
   ...
end

Branches are exclusive, use of elseif can be skipped:

if not room then
   room = currentRoom
   print("Invalid move")
end
if room == 4 then
   print("Congratulations, you won!")
end

EDIT:

I would rearrange the code at the bottom as:

-- Update room state.
room = rooms[currentRoom][move]
if room then
  currentRoom = room
else
  print("Invalid move")
end

-- Reached final state?
if currentRoom == 4 then
  print("Congratulations, you won!")
end

that avoids the currentRoom = room or currentRoom statement (currentRoom only gets updated is room is not nil).

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first answer. \$\endgroup\$ – SirPython Apr 13 '16 at 23:52
  • \$\begingroup\$ Thanks @SirPython. I hope I can engage more and keep learning and improving my skills while reviewing code. \$\endgroup\$ – Diego Pino Apr 14 '16 at 7:03
  • \$\begingroup\$ @DiegoPino Is there a reason why you prefer omitting elseif? IMO elseif is shorter and more importantly makes immediately clear to the reader that only one of the branches will be taken. Intuitively I'd also assume elseif would be more performant because if room is nil, the second check can be skipped. Not that performance would matter in this specific case, but as I see three arguments (brevity, clarity, performance) in favor of elseif and no obvious one for two ifs, I'd argue making elseif a habit seems like a good idea. \$\endgroup\$ – Thomas W. Apr 20 '16 at 5:37
  • \$\begingroup\$ @ThomasW. I think those are good points. My rule of thumb is to use elseif where a switch statement would fit. I think switch is mostly helpful when there are several branches, more than two. In this case I wouldn't use it, but that's mostly a personal preference. \$\endgroup\$ – Diego Pino Apr 20 '16 at 7:29
0
\$\begingroup\$

I see many small mistakes that can be improved upon, Diego Pino did an excellent job of pointing out a few, I'll include his and explain the ones I add in myself:

local rooms = {{south=3,east=2},{south=4,west=1},{north=1,east=4}}
local currentRoom = 1
local room --Room is used every interaction, so I declared it out here
while true do --Changed loop for performance reasons
  room = rooms[currentRoom][io.read()] --Move variable was only used once so I replaced it
  if not room then
    room = currentRoom
    print("Invalid move")
  elseif room == 4 then
    print("Congratulations, you won!")
   break --I break here rather than doing an additional check at the end of our loop iteration
  else
    currentRoom = room --Put this inside and else since it's only necessary if another room is picked
  end
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.