6
\$\begingroup\$

I'm getting started with Python and one of the beginner projects to learn the language was a password generator (one of the dozens already in CodeReview). I have a C# and JS background so I want to make sure I'm doing things the Python way.

Technically the include_punctuation and include_digits properties could be omitted and the exclusion list would be a custom concat of strings.digits+strings.punctuation.

The reasoning behind the PasswordModel is that any component that has an API of charset and length is a valid model. That would mean we could create a MemorablePasswordModel for passwords such as "apple truck butterfly postcard" with a clean and separate model.

from random import choice
from string import digits, ascii_letters, punctuation


def filter_exclusions(items, exclusions):
    result = []
    for i in items:
        if i not in exclusions:
            result.append(i)

    return result


class PasswordModel:
    def __init__(self, length=8, include_symbols=True, include_digits=True, exclusion_list=""):
        self.exclusion_list = list(exclusion_list)
        self.length = length
        self.include_symbols = include_symbols
        self.include_digits = include_digits

    def charset(self) -> str:
        chars = [ascii_letters]

        if self.include_symbols:
            chars.append(punctuation)
        if self.include_digits:
            chars.append(digits)

        return "".join(filter_exclusions("".join(chars), self.exclusion_list))


class PasswordGenerator:
    def __init__(self, password_model=PasswordModel()):
        self.model = password_model

    def generate(self) -> str:
        charset = self.model.charset()
        return "".join(choice(charset) for _ in range(self.model.length))


# Test it out
model = PasswordModel(length=24, exclusion_list="\\/@%")
gen = PasswordGenerator()

print(gen.generate())

Questions

  1. Is there a cleaner way to "include" symbols and digits, I just appended them to a list.
  2. Is there anything built-in to exclude one list from another?
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Is there a cleaner way to "include" symbols and digits, I just appended them to a list.

Cast (rather than wrap) ascii_letters etc. to a set.

Is there anything built-in to exclude one list from another?

Yes - don't use lists; use sets and apply set subtraction.

Otherwise: if you perform a cursory scan of this site for Python-based password generators you will hear some of these same points, but -

  • Do not use random; use the secrets module to generate secure random numbers
  • PasswordGenerator being its own separate class does not make sense; it doesn't offer anything over generate so generate can be a free function or more likely a method on PasswordModel
  • charset would be better to return a set than a str
  • Maybe exclusion_list is being accepted as a string due to the valid concern that mutables (lists, etc.) should not be made parameter defaults. That's fine, though a type hint exclusion_list: Iterable[str] = "" should make it clear that other iterables are acceptable, such as a set or tuple.
\$\endgroup\$
4
\$\begingroup\$

You have written the code carefully, explained your reasoning well, and asked sensible questions. That said, I would encourage you to step back and ask whether the whole thing is over-engineered. The obvious alternative is a single, simple function that has a few keyword arguments and returns the password as a string:

def password(...) -> str:
    ...

How does creating a class called PasswordModel enhance the user's ability to customize behavior? It doesn't, because it's barely more than a data-class -- in other words, it's just a bundle of function kwargs. The one exception to that claim is the charset() method, which gives the user the ability to obtain the universe of characters used in the generation process. Maybe that's useful, maybe not. But let's assume it is. Can we achieve the simplicity of a function-based implementation with the added benefit of providing one or more informational methods or attributes to users, like charset()? Yes, just use a simple class and be sure to define __str__(). Here's the idea as a minimal sketch:

class Password:

    def __init__(self, length, chars = None):
        self.length = length
        self.chars = chars or ascii_letters
        self.password = ''.join(choice(self.chars) for _ in range(self.length))

    def __str__(self):
        return self.password

If your goal is to generalize this to include not just character-based password generation but also word-based generation (eg, gluing together common words into password phrases), you might also want to rethink the terminology: elements rather than chars or charset; n rather than length; and so forth.

But, honestly, I don't care for any of these class-centric approaches. A password is a very simple thing: it's just data. As such, it should be immutable. We could mess around to try to make our Password class quasi-immutable, but things are a lot easier if we go back to a simple function. But instead of returning a string, it could return an immutable data object. For example:

from dataclasses import dataclass

# The data is simple and immutable.

@dataclass(frozen = True)
class Password:
    password: str
    elements: tuple

    def __str__(self):
        return self.password

# The algorithm is just a function, but still customizable.

def password(n, elems = None):
    elems = tuple(elems or ascii_letters)
    password = ''.join(choice(elems) for _ in range(n))
    return Password(password, elems)
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Your Password is a somewhat strange representation. I don't see much use in pairing up a password with the elements from which it was generated. The OP's PasswordModel class accurately represents one concern - criteria for password generation - and needn't be coupled with a totally different concern, which is a single instance of a generated password. \$\endgroup\$
    – Reinderien
    May 13, 2021 at 20:46
  • \$\begingroup\$ @Reinderien I agree: Password.elements is not so useful, but I'm taking the implied goals of the OP's code as a given. Saying that we have a properly defined set of classes, each with their careful boundaries between concerns, is a kind of pseudo-rigor in a case this simple. The more practical and useful boundary is between the immutable data and the algorithmic behavior. If it were me, I'd just have a function returning an immutable string. But if elements is somehow useful information needed later, it can be accommodated simply enough, as illustrated. \$\endgroup\$
    – FMc
    May 13, 2021 at 21:09
  • \$\begingroup\$ @Reinderien To elaborate, my prior comment doesn't adequately convey how much I do agree with your underlying point. If a person wanted to combine your point with the suggestions in my answer, the end result could be a password() function that takes immutable data for password-generation (frozen dataclass) and that returns immutable data for a password (could be str or a frozen dataclass). \$\endgroup\$
    – FMc
    May 13, 2021 at 21:33
  • 1
    \$\begingroup\$ Thank you for the thoughtful response. It is absolutely (and intentionally) over-engineered to try and cram as much Python stuff into a simple project to learn as much as possible, however your criticism is perfectly warranted. This is code review after all! You both have given me a lot to think about going forward and I appreciate the time both of you have taken with this review. \$\endgroup\$ May 14, 2021 at 9:02

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.