1
\$\begingroup\$

This is considered as my second python project, I made the password generator where user has to input length of password minimum 5.

I made the code that it will guarantee at least one lowers case, one upper case and one special character is included in the password. please review and comment.

import random


def letters():
    return list('abcdefghijklmnopqrstuvwxyz')


def upper_case():
    uppercase = []
    for x in letters():
        uppercase.append(x.upper())
    return uppercase


def special_char():
    return list('!@#$%^&*()\'\"')

def all_char():
    return letters() + upper_case() + special_char()

def create_password(password, n):
    for x in range(n):
        password += random.choice(all_char())
    return password

def check_password(password):
    nletter, nupper, nspecial = [0]*3
    for x in password:
        if x in letters():
            nletter += 1
        if x in upper_case():
            nupper += 1
        if x in special_char():
            nspecial += 1
    if nletter > 0 and nupper > 0 and nspecial > 0:
        return True
    else:
        return False


def main():
    n = 0
    while n < 5:
        n = int(input('Enter your password length (minimum 5): '))
    password = ''
    while not check_password(password):
        password = ''
        password = create_password(password, n)
        check_password(password)

    print('Generated password : {}'.format(password))

while True:
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ You could error check the user input, you can never trust people do what you ask. If I enter a letter it crashes. Good to get into the habit of writing comments as you write the code. It works, and that's the important thing \$\endgroup\$ – Matthew Page Jan 24 at 21:02
  • \$\begingroup\$ I added the following try statement ~ while n < 5: try: n = abs(int(input('Enter your password length (minimum 5): '))) except ValueError: print('Enter a valid number') \$\endgroup\$ – AMJ Jan 25 at 19:51
2
\$\begingroup\$

To build on what else has been posted, validating your inputs is necessary and you have a lot of functions and loops that can be removed.

String module

First, I would like to introduce you to the string module that is part of the python standard library. This module has string constants that you can import and use:

>>> import string
>>> string.ascii_lowercase
'abcdefghijklmnopqrstuvwxyz'
>>> string.ascii_uppercase
'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
>>> string.ascii_letters
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
>>> string.punctuation
'!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~'

Using these would allow you to get rid of your first three functions (letter, upper_case, and special_char). Similarly to what Joe said, it would be better to make your character space a variable to avoid creating extra objects, and remove the all_char function at the same time.

create_password function

This function is clear and works okay, but can also be improved. As you are doing the same operation in each iteration of the loop (random.choice) this is and ideal scenario to use a list comprehension.

# Before
def create_password(password, n):
    for x in range(n):
        password += random.choice(all_char())
    return password

# After
def create_password(n):
    return ''.join([random.choice(all_char()) for _ in range(n)])

This works by creating a list where a random character is selected from all_char() and then joining them all together. The underscore is a 'throwaway' variable here, see here.

Alternatively, as we are already using the random module we could use random.choices (but only in python3.6 and above) as this will return a list of k size:

def create_password(n):
    return ''.join(random.choices(all_char(), k=n))

check_password function

In general, simple statements are better than complex statements. Therefore, I would change the first line of this function to be:

nletter, nupper, nspecial = 0, 0, 0

As this is clearer.

The for loop works, and could be improved by using the string module constants, though they would need to be made into lists.

for x in password:
    if x in list(string.ascii_lowercase):
        nletter += 1
    if x in list(string.ascii_uppercase):
        nupper += 1
    if x in list(string.punctuation):
        nspecial += 1

Another way to test if two strings overlap is to use sets; but this is getting more complex.


Altogether

import random
import string

def create_password(n, chars):
    return ''.join(random.choices(chars, k=n)) 

def check_password(password):
    nletter, nupper, nspecial = 0, 0, 0
    for x in password:
        if x in list(string.ascii_lowercase):
            nletter += 1
        if x in list(string.ascii_uppercase):
            nupper += 1
        if x in list(string.punctuation):
            nspecial += 1
    if nletter > 0 and nupper > 0 and nspecial > 0:
        return True
    else:
        return False

def main():
    characters = string.ascii_letters + string.punctuation
    n = 0
    while n < 5:
        try:
            n = int(input('Enter your password length (minimum 5): '))
        except ValueError:
            print('Not a number')

    password = ''
    while not check_password(password):
        password = create_password(n, characters)

    print('Generated password : {}'.format(password))

run = True
while run:
    try:
        main()
    except KeyboardInterrupt:
        run = False
        print('Ctrl+c caught exiting')
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot man! a lot of new subjects to go through and learn I will be looking more into using list comprehension, Can you explain more about KeyboardInterrupt exception \$\endgroup\$ – AMJ Jan 26 at 18:51
  • \$\begingroup\$ Because the main() function is running perpetually (while True:...) you exit your script by pressing Ctrl+c, which raises KeyboardInterrupt. By using try:...except KeyboardInterrupt:... we can handle the programme exiting more gracefully. There are other ways to handle this, such as the module signal, but catching the KeyboardInterrupt works nicely in cases like yours. \$\endgroup\$ – Alex Jan 27 at 12:38
1
\$\begingroup\$

Lots of lists

Every time you call letters, upper_case, special_char or all_char, you are creating a new list which will be no different from the other lists that were previously created. You can make these variables that are assigned once and used everywhere.

Repeated checks

For every password you generate, you are checking its validity twice: once at the end of the loop, and the other on the while line. The former serves no purpose, and should be removed.

Unused assignment

The assignment to password = '' inside your while loop is unused, and can be removed.

\$\endgroup\$
  • \$\begingroup\$ You're welcome. Please don't forget to mark this as best answer if it worked for you. \$\endgroup\$ – Joe C Jan 25 at 22:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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