Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This Python main is what greets the user to a thrice nested super duper barebones OS. The entire rest of my code isn't bug free but this section is, and it feels dirty anyway. I'd like to know how to make it better, by adhering to conventions, or by removal of fatty code.

NOTE: I am not looking to use classes with any of this, because part of the challenge of this program is structuring it with procedural means.

from functions import *

innerLoopFlag = False
while True:
 if innerLoopFlag:
     break
 makeNew = raw_input("Login || Create New Account... ")
 if makeNew == "c-na":
     ui = createAccount()
     createFileSyst(ui)
     break
 if makeNew == "l-a":
     ui = [raw_input("Username... ").strip(),pw("Password... ").strip()]
     if not os.path.isdir(ui[0]):
         print "It appears that this username doesn't exist, please log in legitimately or create an account"
         continue
     if ui:
         while True:
             if takeCheck(ui):
                 print "Successful login..."
                 innerLoopFlag = True
                 break
             else:
                 print "Failure logging in, retrying..."

all()
print "Goodbye", ui[0]
share|improve this question
    
The Python convention for variable names is lowercase_with_underscores. You can find the Python style guide in PEP 8. –  alexwlchan May 12 at 20:53
add comment

2 Answers 2

I don't understand what you mean by "thrice nested", or why you call this an OS. I'm reviewing it as a login prompt.

The entire rest of my code isn't bug free but this section is

Beware of believing something is bug-free. It never is. This is especially important for code which needs to be secure, as this presumably does.

Now for the review:

Many variables need names that say what they mean:

  • What does innerLoopFlag mean? Something like successful_login, right?
  • makeNew: command?
  • ui: credentials?
  • createFileSyst: (There's no need to abbreviate such a rarely used name.) Something like createUserDirectory? But it should be called from createAccount, not here.
  • takeCheck: What does this do? Is it isValidLogin?
  • all: Is it something like runUserSession?

Simplifications:

  • The conditional break at beginning of the loop can become the loop condition: while not innerLoopFlag.
  • The if ui test is always true.
  • The inner loop can be simplified by making the loop condition do the work:

    while not takeCheck(ui):
        print "Failure logging in, retrying..."
    print "Successful login..."
    break
    

    This also makes innerLoopFlag unnecessary. However, why are you retrying at all? Do you expect login to fail and then succeed?

Bugs:

  • Stripping the password is not safe, because passwords can contain whitespace.
  • Security hole: the isdir call lets anyone test for the existence of arbitrary directories. Imagine trying to log in with a username like '../../../some/private/directory'. Also, it depends on the current directory. Is there a user table you could look the user up in instead?
  • (Revealing whether a user exists is also sometimes considered a security hole, but that's debatable.)

Usability:

  • Are "c-na" and "l-a" user-supplied strings, or generated by raw_input, or what?
  • Output ending in "... " looks like a progress message ("Please wait..."), not a prompt. "Login: " and "Password: " are the standard prompts.
share|improve this answer
    
Welcome to Code Review! It's a lovely answer you have here. We need more people like you. –  Simon André Forsberg May 13 at 8:46
add comment

So why not use functions for the steps?

work_to_be_done = True

while work_to_be_done:
    if makeNew == "c-na":
        do_make_account()
        work_to_be_done = False
    elif makeNew == "l-a":
        result = login()
        if result:
            work_to_be_done = False
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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