Skip to main content
2 of 3
Add a forgotten type signature to the example code
Sara J
  • 4.2k
  • 12
  • 37

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'))
Sara J
  • 4.2k
  • 12
  • 37