[Critique Request] Please tear apart my code!

OutlawBlue9, on 21 April 2012 - 07:41 AM said:

@OminousPenguin -

As this program is MOSTLY (although having a neat little user database on my server is also useful) to teach me to program after a 15 year hiatus I welcome any and all suggestions / tips to improve efficiency or what have you! I was going to actually create a thread asking for just pure and simple recommendations such as this as I'm always a fan of limiting the focus of individual threads but thank you for your input none the less. But while we're here lets go down your suggestions:

1) I see your point and calling a function too often with no actual change or results is never a good idea and it definitely seems that way now, such as here:

Spoiler

function home()
homescreen()
if #userlist == 0 and status == "Logged Out"
  then
   admin = true
   function newuser()
   homescreen()
	if admin == true
	 then
	  print("Please register Admin:")
	  write("Enter Username:")
	 else
	  print("Please register User:")
	  write("Enter Username:")
	end

Obviously homescreen() has just been called at the beginning of home() and then immediately after in newuser() without any screen changes, however I plan to add in functionality that will call newuser() (and other such functions) at other times which will require a homescreen() flush so to speak. Granted I may have missed an occurrance that is currently and will forever be superfluous so feel free to cite specific example! Or maybe I just missed your point entirely :)/>/> Aside: It may look strange because it wasn't till after I wrote most of this code that I realized you do not have to call a function immediately after ending it (derp!) which is how every example worked in the tutorial I looked at so the function organization is a bit funky. If I were to re write this code (and I probably will do this at some point) I would have functions such as newuser() and login() defined outside of any other functions.

2) Thank's for the heads up! I had not noticed that (mostly because the code as it was wouldn't even run that far and I was preoccupied with the previous error!) but it is now fixed! login() is now called after the nil check and it works as intended (for the most part; see earlier section to MysticT about new bug…)

3) Valid point. changed the call to x in line 85 to 3-a which is all x is anyways as you point out.

4) I'm guessing you mean that some of my if statements could be made shorter with a for or while loop? Correct me if I'm wrong. If that is the case, in my mind the if statement seems simpler I suppose? But lets challenge that. Give me an example of where you would use a loop instead of what I have but don't say anything about how you would implement and I will challenge myself to make it more efficient by using a loop (best way for me to learn eh?)

Again thanks for your suggestions! I always welcome constructive criticism! Especially early on to help defeat long term bad habits. I might make this into a separate "Tear my code apart!!" thread in order to avoid OT discussions so reply there if you could!

version = "v.01"
userlist = {}
status = "Logged Out"
currentuser = "New User"
a=0
function homescreen()
  term.clear()
  term.setCursorPos(1,0)
  print(string.rep("*", 50))
  print("*Running OutSec "..version.."					  ".. textutils.formatTime( os.time(), false)) --currently displays Minecraft time, not real time
  print("*Status: "..status)
  if status == "Logged In"
   then
	term.setCursorPos(20,3)
	print("User: " ..currentuser)
  end
  print(string.rep("*", 50))
  term.setCursorPos(1,10)
end
homescreen()

function home()
homescreen()
if #userlist == 0 and status == "Logged Out"
  then
   admin = true
   function newuser()
   homescreen()
	if admin == true
	 then
	  print("Please register Admin:")
	  write("Enter Username:")
	 else
	  print("Please register User:")
	  write("Enter Username:")
	end
  
	newusername = io.read()
	write("Enter New Password:")
	newpassword = io.read()
  
	homescreen()
	print("Username: "..newusername)
	print("Password: "..newpassword)
	print("Confirm?[Y/N]")
	local event, k1 = os.pullEvent("key")
  
	if k1 == 21 --y
	 then
	  table.insert(userlist, newusername)
	  userlist[newusername] = {}
	  table.insert(userlist[newusername], "password")
	  userlist[newusername].password = newpassword
	  table.insert(userlist[newusername], "adminstatus")
	  userlist[newusername].adminstatus = admin
	  home()
	 elseif k1 == 49 then --n
	  newuser()
	end
   end
   newuser()

  elseif #userlist > 0 and status == "Logged Out" then
   function login()
	homescreen()
	write("Please enter Username:")
	loginid = io.read()
  
	if userlist[loginid] == nil
	 then
	  print("Invalid Username. Please try again.")
	  sleep(2)
	  login()
	end
   end
   login()
  
   function password()
	homescreen()
	if a>0
	 then
	  term.setCursorPos(1,9)
	  print("Incorrect Password. ("..3-a.."/3 attempts remaining.)")
	end
	write("Please enter password:")
	loginpass = io.read()
	if loginpass == userlist[loginid].password
	 then
	  status = "Logged In"
	  currentuser = loginid
	  print("Entry Authorized.")
	  sleep(2)
	  homescreen()
	 else
	  a = a+1
	  if a < 3
	   then
		password()
	   else
		a=0
		homescreen()
		print("Too many failed attempts. Please wait.")
		sleep(2)
		login()
	  end
	end
   end
   password()
end
  
end
home()

OutlawBlue9, on 21 April 2012 - 08:06 AM said:

1) in homescreen() lines 8-21: I intend to make a border at the top of the screen made out of *. There has to be a better way of the awkward and manual way I go about it here.

2) in homescreen() line 12: I call os.time() and then print it. This however is Minecraft time and I would prefer it to call the realworld time. I've seen and tried some examples (such as one that accesses a php website and takes the time from there) that didn't work so well .

3) In home() beginning at line 48: I ask it for a key input for a Y/N menu which works, but when it goes to the next io.read() whichever key I pressed is there in the command prompt. Not sure why this is??

I'm more looking for big picture things (Some of OminousPenguin's are great examples; use more loops!!!) that fix bad habits but I just put these up here to spur discussion. Thanks in advance!!

OutlawBlue9, on 21 April 2012 - 07:41 AM said:

It may look strange because it wasn't till after I wrote most of this code that I realized you do not have to call a function immediately after ending it (derp!) which is how every example worked in the tutorial I looked at so the function organization is a bit funky. If I were to re write this code (and I probably will do this at some point) I would have functions such as newuser() and login() defined outside of any other functions.

OutlawBlue9, on 21 April 2012 - 07:41 AM said:

Obviously homescreen() has just been called at the beginning of home() and then immediately after in newuser() without any screen changes, however I plan to add in functionality that will call newuser() (and other such functions) at other times which will require a homescreen() flush so to speak. Granted I may have missed an occurrance that is currently and will forever be superfluous so feel free to cite specific example!

local event, k1 = os.pullEvent("char")
if k1 == "y" then
do whatever
end

OutlawBlue9, on 21 April 2012 - 07:34 PM said:

1) Well that sucks. Even the left and right borders? I just feel that adding two extra asterisks and 50 spaces to every line is just so…… inelegant and inefficient…. but oh well. I guess that's why we have higher level programming languages?

for y=1,18
  term.setCursorPos(50,y)
  term.write("*")
end

2) Well I mean it "works" just fine…but the only issue is I would prefer it to display the real world time as opposed to the time it is in Minecraft. eventually I want to use this info to also create error and login logs and having real world time is much more useful in that aspect than having what time it is in Minecraft since a day is only 10 minutes or so…..

3) Ah I see. So would something like this solve the problem?

local event, k1 = os.pullEvent("char")
if k1 == "y" then
do whatever
end