4
\$\begingroup\$

This a fairly basic script I made for use in some other projects, nothing too complex or sophisticated and obviously lacking in some features (Password/Username encryption, Missing file contingencies etc.).

It loads a text file (formatted as USERNAME, PASSWORD\nUSERNAME, PASSWORD\n) into a 2D array, then checks the inputs against it, until 2 separate logins are used.


I'm what I'd consider intermediate in terms of python competency, but there are always things to be improved upon: any suggestions? I tend to use something similar to this for all projects that require a login, so any improvements or areas to look into, no matter how minor, would be greatly appreciated.


def login():
    player_count, details = 0, [[data.strip() for data in line.split(",")] for line in open("username_password.txt", "r").read().splitlines()]
    while player_count != 2:
        username, password = input("USERNAME"), input("PASSWORD")
        for data in details:
            if username == data[0] and password == data[1]:
                print("LOGIN CORRECT")
                details.pop(details.index(data))
                player_count += 1
                break
        else:
            print("INCORRECT")
    main()

def main():
    print("LOGGED IN\nMAIN")

login()
\$\endgroup\$
2
  • \$\begingroup\$ Could your file contain repeated/duplicated username or password? \$\endgroup\$ Oct 28, 2019 at 14:58
  • \$\begingroup\$ Hypothetically yes, thereby bypassing the preventatives in place to stop the same user logging in twice, however when creating the loggins I always have measures to stop such an occurance. \$\endgroup\$ Oct 28, 2019 at 16:36

4 Answers 4

3
\$\begingroup\$

I would split off player_count = 0 to a different line. Multiple assignments on one line is perfectly fine for simple assignments (like a, b = 1, 2), but a double list comprehension doesn't qualify for "simple".

Readability Counts

In general, Readability is far more important than conciseness. In fact, the reason concise code is often better is because it's more readable.

Normally, I'd be very careful with double list assignments for that reason. However, in this specific case, I think making it a single line is a good call. It's not overly long nor complex.

Performance

For pure python, I don't think you can really improve your performance even more. You won't get any issues until your password file grows into the MB's, and if that's a problem, you shouldn't be using a file with plaintext passwords anyway, since you'll be running your software in a production environment.

Instead, you'd be hashing and salting your passwords and storing them in a database, which is coincidentally also the best way to improve your performance if you get datasets that large.

However, as long as your username and password are requested from the user by means of the input() function, you won't have an issue.

dict.pop()

This function removes the value from the list. However, you don't seem to be actually doing anything with that value, and you're also discarding the list. So I'd just drop that line.

The only thing I can imagine you fix it would be to stop player2 from using the same credentials as player1. However, you don't save player references anyway. If you want to stop the same credentials from being used twice, I'd recommend to instead save references to the players for later, and then check that a player hasn't already been logged in as another player.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the suggetions, will revise the topics nessessary for the larger files ill probably encounter at some point. However in my experience, the dict.pop() removes the element from the list in so preventing the user from using the same login again, The code was also pulled from a project that uses the popped element to later refer to in a "current user" sort of way. Thanks for the suggestions, appreciate the help massively! \$\endgroup\$ Oct 28, 2019 at 16:34
  • \$\begingroup\$ Hm, CodeReview is really for code you've written yourself, you know. But yes, that's what you could use it for in that way. \$\endgroup\$
    – Gloweye
    Oct 29, 2019 at 7:00
3
\$\begingroup\$

Advises for optimization:

  • open("username_password.txt", "r").read(). This could be called as "negligent treatment" of the file object. The good and robust way is to always close the file object/resource after usage/consumption.
    We have a convenient feature called context managers (with statement) for "automated" graceful closing of used resource:

    with open('filename') as f:
        content = f.read()
    
  • details = [[...]]. Instead of manually stripping and storing the file contents as list of lists, the more flexible way is using csv.reader object which allows strip of surrounding whitespaces (skipinitialspace=True option) and recognize delimiters.
  • considering that your file represents pairs of USERNAME, PASSWORD, in scope of application we usually expect the usernames to be unique. Therefore having a dict indexed with usernames and passwords as respective values would be an optimized and more performant way - giving us a quick search by usernames (Python dict is a special case of hashtable data structure).
    Besides, it's really not good to remove items from the a list in-place while it's iterated (for data in details: if ... details.pop(details.index(data))), it'll lead to unexpected results. Yes, you have a "circuit breaker" break for that, but still it's better to not get used to fragile approaches.

    with open("username_password.txt") as f:
        creds = {u: p for u, p in csv.reader(f, skipinitialspace=True)}
    
  • dict.get() call allows to flexibly combine check for username/password:

    if creds.get(username) == password:
        ...
    

The final optimized version:

import csv

def login():
    player_count = 0
    with open("username_password.txt") as f:
        creds = {u: p for u, p in csv.reader(f, skipinitialspace=True)}

    while player_count != 2:
        username, password = input("USERNAME"), input("PASSWORD")
        if creds.get(username) == password:
            print("LOGIN CORRECT")
            player_count += 1
            creds.pop(username)
        else:
            print("LOGIN INCORRECT")
    main()


def main():
    print("LOGGED IN\nMAIN")


login()
\$\endgroup\$
2
\$\begingroup\$

Here's a small suggestion -

You could make use of the -

if __name__ == "__main__": guard

Like this -

def login():
    player_count, details = 0, [[data.strip() for data in line.split(",")] for line in open("username_password.txt", "r").read().splitlines()]
    while player_count != 2:
        username, password = input("USERNAME"), input("PASSWORD")
        for data in details:
            if username == data[0] and password == data[1]:
                print("LOGIN CORRECT")
                details.pop(details.index(data))
                player_count += 1
                break
        else:
            print("INCORRECT")
    main()

def main():
    print("LOGGED IN\nMAIN")

if __name__ == "__main__":
    login()
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the suggestion. Have yet to look into any guarding or the like, but will look into it! \$\endgroup\$ Oct 28, 2019 at 16:30
1
\$\begingroup\$

I tend to use something similar to this for all projects that require a login

Then it's time to stop using self-rolled unencrypted authentication. It's fine as a one-off proof-of-concept, but as you've described, that's not how it's actually being used.

Read through https://stackoverflow.com/questions/7014953/i-need-to-securely-store-a-username-and-password-in-python-what-are-my-options - specifically, consider using something like keyring. Not only will it be more secure - it will also simplify your code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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