9 replies to this topic

[Kizz]

Greetings Fellas!

I am once again bored at work, doing a bit of programming. I am making a little program to allow me to switch the states of a few objects from on to off and back on. As I have almost always programmed the same way for the last few years, I would like a few suggestions on improving my code.

The functional part of this code hasn't been created. I have no issues doing that. What I would like to know is: Is there a better, more compact or more correct way to build the menu I have created. Is what I have solid, or am I missing a very obvious alternative that could improve my code?

Here is my code:

--defaults
alrm=1
lck=0
light=1
function getAlrm()
if alrm==1 then
  alrm=0
  return
end
if alrm==0 then
   alrm=1
   return
end
end
function getLck()
if lck==1 then
  lck=0
  return
end
if lck==0 then
  lck=1
  return
end
end
function getLight()
if light==1 then
  light=0
  return
end
  if light==0 then
  light=1
  return
end
end
while true do
--::home::
if alrm==1 then
alrmState="Disable"
end
if alrm==0 then
alrmState="Enable"
end
if lck==1 then
lckState="Disable"
end
if lck==0 then
lckState="Enable"
end
if light==1 then
lightState="Disable"
end
if light==0 then
lightState="Enable"
end
print'Welcome to Base Control'
print'Please make a selection:'
  print('1: '..alrmState..' Night Alarm')
  print('2: '..lckState..' Base Lockdown')
  print('3: '..lightState..' Base Lighting')
  print'4: Exit'

print''
write'Selection:'
rInput=read()
rInput=tonumber(rInput)

if rInput==1 then
print(''..alrmState..'ing alarm!')
getAlrm()
end
if rInput==2 then
print(''..lckState..'ing lockdown!')
getLck()
end
if rInput==3 then
print(''..lightState..'ing lights!')
getLight()
end
if rInput==4 then
print'Closing!'
return
end
if rInput~=1 and rInput~=2 and rInput~=3 and rInput~=4 then
print'Invalid choice'
end
sleep(1)
term.clear()
term.setCursorPos(1,1)
end

Alternatively, here is the pastebin: http://pastebin.com/b8HPJCKE

Again, there is nothing functionally wrong with the menu. It works as intended.

Edited by Kizz, 11 June 2015 - 01:34 PM.

[KingofGamesYami]

I'd do something along these lines:

local alrm = true
local lck = false
local light = true

...and because they are boolean, I can do stuff like this:

  print('1: ' .. (alrm and "Disable" or "Enable" ) .. ' Night Alarm')

Removing the need for this part of the code

if alrm==1 then
alrmState="Disable"
end
if alrm==0 then
alrmState="Enable"
end

This can be applied to the other variables as well.

Then, with boolean variables, this:

function getAlrm()
if alrm==1 then
  alrm=0
  return
end
if alrm==0 then
   alrm=1
   return
end
end

...is as simple as this:

function getAlrm()
  alrm = not alrm
end

[MKlegoman357]

Also, you should use 'else' and 'elseif' instead of just 'if's:

if rInput==1 then
  print(''..alrmState..'ing alarm!')
  getAlrm()
elseif rInput==2 then
  print(''..lckState..'ing lockdown!')
  getLck()
elseif rInput==3 then
  print(''..lightState..'ing lights!')
  getLight()
elseif rInput==4 then
  print'Closing!'
  return
else
  print'Invalid choice'
end

You could improve the overall readability by indenting the code.

[Kizz]

Thanks guys! After posting this I did consider booleans but wanted to wait for suggestions. Never seen the ability to classify based on a boolean in one string. Pretty neat!

And yea, I often am too lazy to use elseif and the lot. Bad habit!

Thanks for the suggestions!

[Lignum]

One thing that also makes your code hard to read is your use of magic numbers. These are random numbers in your code. Consider making constants for them or at least commenting the lines which make use of the numbers.

if rInput==1 then
  print(''..alrmState..'ing alarm!')
  getAlrm()
elseif rInput==2 then
  print(''..lckState..'ing lockdown!')
  getLck()
elseif rInput==3 then
  print(''..lightState..'ing lights!')
  getLight()
elseif rInput==4 then
  print'Closing!'
  return
else
  print'Invalid choice'
end

For example this code could be improved by making constant variables for the numbers 1, 2, 3 and 4. As the reader, it's hard for me to understand what these numbers mean (which is the main issue of using magic numbers), but I can give you an example based on context:

local INP_ALARM = 1
local INP_LOCKDOWN = 2
local INP_LIGHT = 3
local INP_CLOSING = 4

if rInput == INP_ALARM then
  print(''..alrmState..'ing alarm!')
  getAlrm()
elseif rInput == INP_LOCKDOWN then
  ...

As you can see, this is much easier to understand.

You could also improve the naming of your functions. getAlrm suggests to me that it'd return the current alarm state, while its code suggests a toggle. So why not call it toggleAlarm?
By using booleans you can also shorten the toggle code to:

local function toggleAlarm()
   alrm = not alrm
end

While it was indirectly addressed by Yami, you should also make all your variables (including functions! remember, functions are variables) local. Check out this post; you can see that the locals have performed better since they don't need to index the global table. Additionally, other programs can access globals from your program. While this may not be a security issue directly, you could be sharing the same variable names as another program and thus strange bugs will arise.

if rInput~=1 and rInput~=2 and rInput~=3 and rInput~=4 then
print'Invalid choice'
end

This code could be shortened to:

if rInput < 1 or rInput > 4 then
   print 'Invalid choice'
end

Anyway, good luck with your program!

[Kizz]

Thanks! As well, I have known and dealt with the local vs global issue before. Very often with my networking and communication programs. (As well in my KOS)

I have a terrible habit of forgetting to use local or just adding it in much later than I should. I had no idea that forcing local would improve performance, but it makes total sense!

Again, thanks for the tips. I look forward to releasing more programs with these tips in mind.

Edited by Kizz, 11 June 2015 - 05:52 PM.

[Kizz]

Alright gentlemen and gentleladies, I have slaved hard for over 15 minutes and compiled all the suggestions into something beautiful.

Let me know what you all think!

--Default Variable Settings--
local alrm=true
local lck=false
local light=true
local function cleanup()
term.clear()
term.setCursorPos(1,1)
end
--Object Manipulation Functions--
local function toggleAlrm() --Alarm
alrm = not alrm
end
local function toggleLck() --Locks
lck = not lck
end
local function toggleLight() --Lights
light = not light
end
--Setup--
cleanup()
--MainProgram--
while true do
--Present Choices--
print'Welcome to Base Control'
print'Please make a selection:'
  print('1: '.. (alrm and "Disable" or "Enable" ) ..' Night Alarm')
  print('2: '.. (lck and "Disable" or "Enable" ) ..' Base Lockdown')
  print('3: '.. (light and "Disable" or "Enable" ) ..' Base Lighting')
  print'4: Exit'

print''
--Pull Input--
write'Selection:'
local rInput=read()
rInput=tonumber(rInput)

--Get States--
if alrm then
  alrmState="Disable"
elseif not alrm then
  alrmState="Enable"
end

if lck then
  lckState="Disable"
elseif not lck then
  lckState="Enable"
end

if light then
  lightState="Disable"
elseif not light then
  lightState="Enable"
end

--Menu Value Masks--
local chAlrm=1
local chLck=2
local chLight=3
local chQuit=4

--Process Choice-- 
if rInput==chAlrm then
print(''..alrmState..'ing alarm!')
toggleAlrm()
elseif rInput==chLck then
print(''..lckState..'ing lockdown!')
toggleLck()
elseif rInput==chLight then
print(''..lightState..'ing lights!')
toggleLight()
elseif rInput==chQuit then
print'Closing!'
return
else
print'Invalid choice'
end

--Cleanup--
sleep(1)
cleanup()
end

Pastebin: http://pastebin.com/JH6K4GkE

[Dragon53535]

--Get States--
if alrm then
  alrmState="Disable"
elseif not alrm then
  alrmState="Enable"
end
if lck then
  lckState="Disable"
elseif not lck then
  lckState="Enable"
end
if light then
  lightState="Disable"
elseif not light then
  lightState="Enable"
end

Can be shortened to

local alrmState = (alrm and "Disable" or "Enable")
local lckState = (lck and "Disable" or "Enable")
local lightState = (light and "Disable" or "Enable")

Note that it's the same as your print statements. You can use them for variable assignment as well as printing. Also, MAKE THEM LOCALS

Edited by Dragon53535, 13 June 2015 - 05:57 AM.

[Bomb Bloke]

See here for some details on why the construct Dragon mentions works as it does.

Anyway, I'm not going to ignore the elephant in the room. I'm talking about tables, of course.

Spoiler

--Default Variable Settings--

local menuItems = {
	{["menuText"] = ' Night Alarm',   ["selectionText"] = 'ing alarm!',    ["state"] = true},  -- 1
	{["menuText"] = ' Base Lockdown', ["selectionText"] = 'ing lockdown!', ["state"] = false}, -- 2
	{["menuText"] = ' Base Lighting', ["selectionText"] = 'ing lights!',   ["state"] = true}   -- 3
}

--MainProgram--

while true do
	term.clear()            -- Let's not make functions to perform tasks we only perform at one point in the code.
	term.setCursorPos(1,1)  -- Don't fall into the habit of using functions as comments, either.
	
	--Present Choices--
	print'Welcome to Base Control'
	print'Please make a selection:'

	for i = 1, #menuItems do  -- '#menuItems' refers to the number of entries in 'menuItems'
		print(tostring(i)..": "..(menuItems[i].state and "Disable" or "Enable")..menuItems[i].menuText)
	end
	
	print(tostring(#menuItems+1)..': Exit\n')  -- The '\n' escape character triggers a line break when used with print.

	--Pull Input--
	write'Selection:'
	local rInput=tonumber(read())  -- No need to call tonumber() on a separate line!
		
	if rInput then  -- tonumber() returns nil if it can't do the conversion, so do an existence check.
		if rInput > 0 and rInput < #menuItems+1 then  -- Toggle something in the table.
			print((menuItems[rInput].state and "Disable" or "Enable")..menuItems[rInput].selectionText)
			menuItems[rInput].state = not menuItems[rInput].state
		elseif rInput==#menuItems+1 then
			print'Closing!'
			return
		else print('Invalid choice') end
	else print('Enter a number!') end

	sleep(1)
end

Suddenly the script becomes a lot shorter, and adding new menu entries becomes a lot easier. You could also expand this so that each menu item has a "side" attached to it, and toggling that item would output its new state out that side of the computer as a redstone state...

[Kizz]

OHH TABLES! I forgot about arrays! (Tables whatever...) Good suggestion! And yes Dragon! I totally realized after I released the update but was way too lazy to fix it haha.

Thanks for the suggestions fellas! I love this community.