9
\$\begingroup\$

I am a self taught python developer and I trying to write clean code as mush as possible. Since I do not have any environment that can give me constructive criticism, I would like to hear your opinions, so, here it is a python password generator, I know that something like that can be made with single function, but this is just "project as a placeholder" if that make sense.

So does this consider "good practice"? And what can I do better?

import sys
import yaml
import string
import random
from typing import Union


class GeneratePassword:
    """
    Object that generate password. Password size is taken from yaml config file,
    it can be week(64), medium(128) or strong(256). Need to pass that as
    string argument to cls.generate_password function.
    """
    @classmethod
    def generate_password(cls, password_level: str):
        conf = cls.check_config_file()
        if conf is not None:
            security_level = conf.get(password_level)
            new_password = cls.generate_gibberish(security_level)
            return new_password

    @staticmethod
    def check_config_file() -> Union[dict, None]:
        try:
            with open('./config/security_level_conf.yml') as pass_conf:
                current_data = yaml.safe_load(pass_conf)
                return current_data
        except FileNotFoundError:
            sys.exit('Config file not found.')

    @staticmethod
    def generate_gibberish(security_level: int) -> str:
        choices = string.punctuation + string.ascii_letters + string.digits
        password = []
        for i in range(security_level):
            character = random.choice(choices)
            password.append(character)
        return "".join(password)


if __name__ == '__main__':
    print(GeneratePassword.generate_password('week'))
\$\endgroup\$
4
  • 2
    \$\begingroup\$ side note, it's weak, not week :) \$\endgroup\$ Commented Oct 7, 2021 at 8:47
  • 1
    \$\begingroup\$ yea, it's a typo that sticks xD \$\endgroup\$ Commented Oct 7, 2021 at 9:08
  • 3
    \$\begingroup\$ Both RFC 4086 and 1password recommend about 96 bits of entropy for passwords. Using your alphanum + ascii punctuation, that's about 15 characters. Even your "weak" passwords at 64 characters (>400 bits) are way overkill. It makes no sense to have the higher strengths. If this is not hypothetical code, that would allow removing the configuration code (and its related error handling etc). \$\endgroup\$
    – marcelm
    Commented Oct 7, 2021 at 14:20
  • 1
    \$\begingroup\$ I was not planning to use it. I agree that is a overkill, but it was just a "project to code" since I did not have any idea what to code, I just wanted to code something that is somewhat "clean" and practice that... \$\endgroup\$ Commented Oct 7, 2021 at 14:24

6 Answers 6

17
\$\begingroup\$

It's clean and readable, the logic is sensible and easy to follow, and I do like seeing type signatures

I do think the structure can be improved, however - a class with only @classmethods and @staticmethods usually doesn't want to be a class at all - if there is no situation where we'd want an instance of the class, or if there is no situation where two instances would be different, we don't really have a class, we just have a bag of functions. Am I saying we should delete the class, or is there something we can do to make the class more meaningful? Well, let's think about that while we look at the functions

check_config_file is well written, but does lose some flexibility by hardcoding the file path. It might be nice if that path could be passed as a parameter instead

generate_password also has a minor annoyance - if we want to generate a lot of passwords, each call to generate_password will open the config file. Not a problem if we're generating ten passwords, might be a problem if we want to generate ten million - we probably won't, but still. It would be nice if we has a place to store the config so we didn't have to look it up each time

So, we want to store state (maybe multiple different states even), and we want to have that state influence our behaviour. That does sound kind of like a class to me - we could have each instance constructed based on a configuration file which it loads just once, and then keep using that configuration to generate as many passwords as we want. Neat

Oh, and check_config_file should probably not be responsible for forcibly shutting down the entire program when it doesn't find a file. Someone might want to call it as part of a bigger program some day, and that'd be easier if this function would announce that it's failed and the caller should handle it (by letting the exception get raised) than if it demanded the entire program shuts down no matter what the caller wants

Finally, some minor nitpicks, mostly about names

  • check_config_file sounds like the name of a function that returns a Bool. What we're doing here is loading and parsing the file, right? So a name like load_config might be a better fit
  • A class name should generally be a type of thing. If someone hands you a thing that generates passwords and asks what it is, responding with "It's a GeneratePassword" does sounds less natural than "It's a PasswordGenerator" I'd say
  • Union[whatever, None] can be written as Optional[whatever] instead, which I feel communicates the intent a bit clearer
  • "week" should probably be "weak" - the latter is the opposite of strong, the former is 7 days
  • Calling generate_gibberish's parameter security_level feels a bit strange - it sounds vague enough that I'd expect it to be able to affect all kinds of stuff like which characters can appear, whether characters can repeat, etc. But it's just the password's length - just calling the parameter length might make that a bit more apparent, giving people a better idea of what's going on
  • generate_password should state that it returns a str

If we were to do all that, we might end up with something like

import yaml
import string
import random


class PasswordGenerator:
    """
    Object that generate password. Password size is taken from yaml config file,
    it can be weak(64), medium(128) or strong(256). Need to pass that as
    string argument to cls.generate_password function.
    """
    def __init__(self, config_path: str):
        with open(config_path) as config_file:
            self._config = yaml.safe_load(config_file)


    def generate_password(self, password_level: str) -> str:
        security_level = self._config.get(password_level)
        new_password = self.generate_gibberish(security_level)
        return new_password


    @staticmethod
    def generate_gibberish(length: int) -> str:
        choices = string.punctuation + string.ascii_letters + string.digits
        password = []
        for i in range(length):
            character = random.choice(choices)
            password.append(character)
        return "".join(password)


if __name__ == '__main__':
    try:
        generator = PasswordGenerator('./config/security_level_conf.yml')
    except FileNotFoundError:
        sys.exit('Config file not found.')

    print(generator.generate_password('weak'))

Update: Alternative approaches

It was pointed out to me in a comment that the class I'd made contains only one non-__init__ method, which is often a sign of an unnecessary class. I agree that that is often the case, and while I would argue that it might be justified in this particular case, other approaches are worth looking at

A single function

Often, a single-method class can be simplified to a single function, and if some of its arguments need to be saved functools.partial can be used. A bit like:

def generate_password(config_file: str, strength: str) -> str:
    with open(config_path) as config_file:
        return generate_gibberish(yaml.safe_load(config_file)[strength])

# Example usage
cfg = functools.partial(generate_password, config_file_path)

for _ in range(100):
    print(cfg("medium"))

Unfortunately, this runs into the same issue as the original code, in that it needs to re-open and re-parse the config file each time it generates a password. This is a nice helper function, but shouldn't be the entire API on its own

Configuration as message

A natural way around that issue would be to instead have two separate functions - one to load the configuration, another which accepts a configuration and a strength. That might look like this:

def load_config(config_path: str) -> dict[str, int]:
    with open(config_path) as config_file:
        return yaml.safe_load(config_file)

def generate_password(config: dict[str, int], strength: str) -> str:
    return generate_gibberish(config[level])

# Example usage
cfg = load_config(config_file_path)

for _ in range(100):
    print(generate_password(cfg, "medium"))

Semantically, this is a very nice approach. It does have one drawback however - it makes the structure of the config into part of the module's interface. Right now we promise that the functions will only produce and consume ints. If we want to change that and add a more complex kind of security level some day, that would require an annoying amount of care to not break the API. I consider that kind of restriction enough of a drawback that I would favor another option

Configuration as callable

We can keep similar semantics without tying our public interface to our internals. Making the configuration into functions can keep the internals flexible by not exposing them:

def load_config(config_path: str) -> Callable[[str], str]:
    with open(config_path) as config_file:
        lengths = yaml.safe_load(config_file)
    
    def generate_password(strength: str) -> str:
        return generate_gibberish(lengths[str])
    
    return generate_password

# Example usage
cfg = load_config(config_file_path)

for _ in range(100):
    print(cfg("medium"))

This keeps our internals flexible, maintains easy-to-understand semantics, and generally avoids the drawbacks of the earlier approaches. Though admittedly, we now have to start worrying about how to turn a configuration into YAML

This approach is similar to the original - if the original one had used __call__ instead of a named method, it would've had almost exactly this interface. By not using a class it's a bit simpler, though if we want to extend it further (say, by adding the ability to check if "weak" in cfg), the class-based original may be easier to work with. Or is there's another way we can get that?

Configuration as name-generator mapping

There's no reason using functions has to mean we can't also use a dict, right?

def load_config(config_path: str) -> dict[str, Callable[[], str]]:
    with open(config_path) as config_file:
        lengths = yaml.safe_load(config_file)
    
    return {strength: functools.partial(generate_gibberish, length) for (strength, length) in lengths.items()}

# Example usage
cfg = load_config(config_file_path)

for _ in range(100):
    print(cfg["medium"]())

I do like this approach. It's easy to implement and gets some useful functionality by default that the original doesn't (like being able to check if "weak" in cfg, or get a list of strength levels with cfg.keys()). Those are pretty strong argument in favor of this approach

But while load_config(config_file_path)["weak"]() is easy to understand, I have to say I don't much like the aesthetics. That's less important than the extra functionality of course, but then again there's nothing stopping you from adding that functionality to a class-based approach if you want.

Is there a conclusion?

The basic two-function approach ties your API to your internals, but isn't bad otherwise I guess. I'd probably recommend the dict-of-functions approach, it's simple, powerful and intuitive. A class-based approach works pretty well still (and makes for a less ugly interface)

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you for this and for your time. I read it couple of times and make sense. I will try to make coding decisions like this next time :) \$\endgroup\$ Commented Oct 7, 2021 at 7:13
  • 2
    \$\begingroup\$ Unfortunately your class with a single method isn't really much better, in terms of design, than OP's code. Both are pseudo-classes that should be replaced by regular functions. See Stop writing classes by Jack Diederich. \$\endgroup\$ Commented Oct 7, 2021 at 16:58
19
\$\begingroup\$

Please

don't

use

random

for

password

generation

in

Python.

Use secrets like this. This has been covered (again, and again, times ?n?) on CodeReview, so I strongly suggest that you read through these links.

\$\endgroup\$
1
  • \$\begingroup\$ I didn't know about secrets, now I know and will check it out. Thanks. \$\endgroup\$ Commented Oct 7, 2021 at 7:11
8
\$\begingroup\$

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))
\$\endgroup\$
2
  • \$\begingroup\$ Nice! My goal here was to do it i OOP way, since I will have coding chalenge for the next job soon. I didn't know about using generators in return statement. I like it. Thanks :) \$\endgroup\$ Commented Oct 7, 2021 at 7:17
  • 1
    \$\begingroup\$ @TheOriginalOdis The code you wrote is not OOP, and the task you chose isn't a good one to practise OOP (because an OOP solution is worse than a non-OOP solution). If you want to practise OOP, do something like implementing data structures (queues, stacks) or a game with different types of in-game characters \$\endgroup\$
    – minseong
    Commented Oct 9, 2021 at 10:35
2
\$\begingroup\$

Classes shouldn't be used when you have no intention of ever creating objects of that type. Classes are not namespaces. In Python, modules are namespaces. Thus all of the code belongs in a module.

The password generator should refer to configuration data in a dict, not reading the file directly.

The check_config_file function seems far-fetched: you assume that anyone using this module will want to store the configuration in exactly the way you specify. If anything, I'd make a simple configuration hierarchy: the module would have a module-level default configuration. It would also have a function that would re-initialize that configuration with one loaded from a file in some standard location. The password generator function would use the configuration from an optional parameter if provided, and fall back to module-level configuration - which could be a default or one loaded from a config file.

All this can be done in the same number of lines of code, so seems like it'd be a usability improvement without adding any complexity.

\$\endgroup\$
1
\$\begingroup\$

You wanted to write OOP code to practise OOP.

Unfortunately, your code is not OOP. You need more than a class keyword to actually be OOP, you need to instantiate objects from the class (your code doesn't have objects) and normally the objects interact with each other. You may misunderstand what objects are since you wrote the docstring "Object that generate password" on a class, not an object. And you never turn the class into an object.

Sara J's answer points out how you could have solved the problem in an OOP way:

class PasswordGenerator:
    def __init__(self, config_path: str):
        self.configuration = ... # set up configuration from YAML file

    def generate_password(self, password_level: str) -> str:
        ... # use self.configuration to generate a password

And you use it like this:

simple_generator = PasswordGenerator('simple_config.yml') # instantiate PasswordGenerator
print('weak password:', simple_generator.generate_password('weak'))
print('strong password:', simple_generator.generate_password('strong'))

simple_generator is an object that has been instantiated from the PasswordGenerator class. simple_generator has its own data (self.configuration in this case) and methods, which is why it's an object — it has fields unique to itself.

A good beginner project to learn OOP is to make your own classes for some datatypes (e.g. queue or stack) and then use them.


This password generator is best solved without using OOP. The most Pythonic solution would just be a module with top-level functions. Yours is already a module — given away by the fact you have only static methods — except you have put the functions into a class for no reason; this is not Pythonic.

Just get rid of class GeneratePassword: and your method decorators @staticmethod and @classmethod, make your static methods top-level functions, and you end up with a Pythonic solution to your password generator task. Then other code that depends on the methods of your module can just import this file as a regular module.

"""
Generates passwords according to YAML configuration.
...
"""
import ...


def generate_password(password_level: str) -> str:
    ...


def check_config_file() -> dict:
    ...


def generate_gibberish(security_level: int) -> str:
    ...


if __name__ == '__main__':
    ...

Your check_config() method seems to be a "private" method that shouldn't be called by users of your code. In Python you should prefix it with an underscore to indicate so: _check_config is a better name if it truly is private. However, in a better-designed implementation (i.e. a module instead of a class) this function shouldn't exist. Just make the config an extra argument of the password generator function directly.


def check_config_file() -> Union[dict, None]:
    try:
        with open('./config/security_level_conf.yml') as pass_conf:
            current_data = yaml.safe_load(pass_conf)
            return current_data
    except FileNotFoundError:
        sys.exit('Config file not found.')

A better way to specify Union[dict, None] is Optional[dict] (also from the typing module).

However, this method either returns a dict (current_data) or ends the program before returning (sys.exit). It never actually meaningfully returns None (unless yaml.safe_load can return None?), so the typehint should just be -> dict.

And you shouldn't use sys.exit within module functions like this. Just throw an error and let the caller deal with it. In your case you can just let any original errors bubble up to the caller:

def check_config_file() -> dict:
    with open('./config/security_level_conf.yml') as pass_conf:
        current_data = yaml.safe_load(pass_conf)
        return current_data

now the caller has to deal with any errors that might arise, and it gets to decide if it truly wants to exit the whole program, or retry, or alert the user via another method.

In more complex cases you can also create your own exception type and throw it when there are issues. Then the caller will catch your own exception type, which could be more specific and/or contain more details pertinent to the problem domain.

\$\endgroup\$
1
\$\begingroup\$

Why use a class?

  1. There is no __init__, no state to be stored.
  2. All "methods" are class methods or static methods.

Config-determined behaviour

All parametrization comes by interpreting configuration from a config file in the methods. This is not good as it reduces reusability of those and creates side-effects (i.e. on the content of the config file).

Instead, use general functions that take arguments to control their behaivour. You can still pass the parsed config values to those then.

KISS / YAGNI

Keep it simple and stupid. Don't overengineer. You ain't gonna need it.

Here's one implementation of a simple password generator, I use in many of my programs:

from secrets import choice
from string import ascii_letters, digits


__all__ = ['genpw']


def genpw(*, pool: str = ascii_letters + digits, length: int = 32) -> str:
    """Generates a random password."""

    return ''.join(choice(pool) for _ in range(length))
\$\endgroup\$

Your Answer

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

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