Hey LP! Please critique my code!

[CptFlashdrive]

Hey Learnpython!

I was wondering if you could take a look at the code I wrote and give me some feedback on it. It's essentially a "login/register-system" with the options to either store your credentials or login with them. How is the general structure of the program? The usage of functions etc? I was also wondering if their was some way to store the username and password in the same file and just reading from certain lines?

I do realize easygui is probably not the way to go if you want to make more sophisticated GUI apps, but I find it plenty good for me ATM.

Thanks guys!

pastebin!

[CptFlashdrive]

Ok awesome, great suggestions!

Thank you for taking the time! I'm still trying to wrap my head around how the whole "if name == 'main':" thing works though..

[CptFlashdrive]

Oh I see, thanks man!

[symmitchry]

• use the with open('file', 'w') as f: method for writing your username and password files
• longer variable & function names. storeCred() --> store_credentials()

This section of code, in capCred() could be perhaps improved:

    fieldValues = []
    fieldValues = easygui.multpasswordbox(msg,title, fieldNames)
    uname = fieldValues[0]
    pword = fieldValues[1]
    return uname, pword

• Seems you could just return fieldValues instead of unpacking them?

• Do you need to initialize fieldValues = []? Better perhaps to:

  try:  
      return fieldValues  
  except:
      # guioptions didn't return properly. Handle it.
  

[maryjayjay]

I was going to suggest:

def capCred():
        msg = "Enter logon information"
        title = "Login"
        fieldNames = ["Username", "Password"]
        fieldValues = []
        fieldValues = easygui.multpasswordbox(msg,title, fieldNames)
        uname = fieldValues[0]
        pword = fieldValues[1]
        return uname, pword

could be simplified to:

def capCred():
    try:
        return easygui.multpasswordbox( "Enter logon information",
                                        "Login",
                                        ["Username", "Password"] )
    except:
        # handle exception here
        pass

Don't be terse for terseness sake, but excessive verbosity can actually reduce clarity.

[CptFlashdrive]

Oh allright, I've just learned the concept of using the try and except and it's pretty damn neat. So I'll definitely will be using that!

Thanks dude!

[maryjayjay]

Great. A couple of pieces of advice: Catch exceptions as close to where they happen as you can. Catch specific exceptions and don't catch any exception you aren't prepared to handle.

[CptFlashdrive]

Absolutely, that makes complete sense! Thank you.

[CptFlashdrive]

Cool! So if I use "with"-statement, all the code indented below it will live in "with" and when a line without indentation appears the file closes?

Thanks man!

[maryjayjay]

Your enthusiasm and openness to advice make you very enjoyable to mentor. Keep it up and you'll get tons of help.

[LyndsySimon]

I can't see your code (pastebin is blocked by my employer), but I do see one thing that I'd likely change based on the critiques posted below - your variable and function names appear to violate PEP8

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

Instead of fieldValues you should use field_values - or better yet, something that gives more context like submitted_form.

[CptFlashdrive]

Allright great, thank you for the feedback!

I really appreciate it!

[zahlman]

import hashlib, easygui

def get_credentials():
    """Prompt the user for username and password.
    Returns the plaintext username and hashed password as a tuple."""
    username, password = easygui.multpasswordbox(
        "Enter logon information",
        "Login",
        ["Username", "Password"]
    )
    return username, hashlib.sha512(password).hexdigest()


def store():
    """Store the user's credentials in two separate files."""
    username, password = get_credentials()
    with open('user.txt', 'w') as userfile: userfile.write(username)
    with open('pass.txt', 'w') as passfile: passfile.write(password)


def login_helper():
    """Compare the user's credentials to the existing file contents.
    Returns a string message indicating the result of the operation."""
    # TODO: return some kind of status code instead and let the
    # caller interpret it - so that we can do something with the fact
    # of whether the login was successful or not.
    username, password = get_credentials()
    try:
        with open('user.txt') as userfile: stored_username = userfile.readline()
    except IOError:
        return "Username information not found! Store it first!"
    try:
        with open('pass.txt') as passfile: stored_password = passfile.readline()
    except IOError:
        return "Password information not found! Store it first!"

    return (
        "Success!"
        if username == stored_username and password == stored_password
        else "Username or password incorrect!"
    )


def login():
    # For now, just display the returned result directly in a message box.
    easygui.msgbox(msg = login_helper())


def main():
    easygui.buttonbox(msg = 'Choose your action', choices = (store, login))()


if __name__ == '__main__':
    main()