Skip to main content
2 of 2
added 663 characters in body
riskypenguin
  • 3.5k
  • 1
  • 10
  • 28

Good use of type annotations!

Variable and method naming is mostly concise and readable.


password_level: week should be weak.


In its current implementation GeneratePassword does not need to be a class, functions would suffice. Consider this current usage inside of another module:

import password_generator
password_generator.GeneratePassword.generate_password("medium")

# or

from password_generator import GeneratePassword
GeneratePassword.generate_password("medium")

versus this more simple example:

import password_generator
password_generator.generate_password("medium")

# or

from password_generator import generate_password
generate_password("medium")

I'd say the GeneratePassword class is unnecessary clutter here. Encapsulation is already achieved by the code being in a dedicated named module.


check_config_file should be named something like load_config_data.

Using sys.exit instead of throwing an error makes the module hard to use within other modules / projects as it will halt all current code execution, as you can check with this simple example:

from password_generator import GeneratePassword

# './config/security_level_conf.yml' is non-existent

GeneratePassword.generate_password("medium")

print("this will not be executed")

Instead pass the config-filepath as an argument / class attribute and let the calling code handle the FileNotFoundError. Alternatively you could use some default config values to handle a non-existent config file.


generate_gibberish

generate_gibberish might be more accurately named something like random_string. generate_gibberish returns password, naming discrepancies like this are usually a hint that at least one of the two isn't named correctly.

security_level would be more accurately named length.

choices should be a constant, it does not need to be re-constructed everytime generate_gibberish is called.

for _ in range(...) is a naming convention used for variables whose value is never needed / used.

Generally try to avoid this pattern of list creation:

xs = []
for _ in range(number):
    xs.append(x)

Instead use a list comprehension:

xs = [x for _ in range(number)]

password = [random.choice(choices) for _ in range(security_level)]

It's more concise, less error-prone and often times faster.

We can also pass the generator expression directly to str.join without constructing the entire list first:

return "".join(random.choice(choices) for _ in range(security_level))
riskypenguin
  • 3.5k
  • 1
  • 10
  • 28