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 @classmethod
s and @staticmethod
s 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 int
s. 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)