One of the easiest ways to improve code is to reduce repetition. The
most-repeated elements in your code are random.choice
and outputpass
. You
can reduce bulk by importing what you need rather than importing a module containing
what you need.
from random import choice
from string import digits, ascii_letters, punctuation
Another bulk-reducer is to choose more practical variable names. By practical,
I mean names that are are more
compact but no less informative to the reader. For example, in a script that
generates a password, the context is quite clear, so you can get away with a
very short variable name: pw
is one sensible option. Similarly, a simple name
like n
is just as clear as asklenght
, because in context everybody
understands what n
means.
On the subject of naming, what does asku
mean? Not very much to me. But
your input prompt text is very clear. A better name might be be method
(which your text uses)
or perhaps even chartype
, which is more specific.
Your code has a bug for All
: it creates a password 3x as long as it should be.
You don't need any special logic for n
of 1, 2, or 3. Just set up
the loop and let it handle everything. The key is to use n
rather
than n - 1
.
for i in range(n):
...
The conditional logic inside the loop has only one purpose: selecting
the relevant characters to include in the password. Anytime you have
a situation like that, logic can often be greatly simplified by
creating a data structure.
characters = {
1: digits,
2: ascii_letters,
3: punctuation,
4: digits + ascii_letters + punctuation,
}
That change drastically reduces the code inside the loop:
pw = ''
for i in range(n):
pw += choice(characters[chartype])
If you want to impress your friends, you can even write it
in one shot by using a comprehension:
pw = ''.join(choice(characters[chartype]) for i in range(n))
For usability, you might also consider changing the way that chartype
works: instead of asking users to type a number, which requires a small
mental translation from a meaningful thing (letters, numbers, symbols, all)
to an abstract thing (1, 2, 3, 4), you could just let them type the
first letter of the actual thing.
Also for usability, if the context is already clear, shorter messages
are easier on users, because they can scan them very quickly.
A final change to consider is whether to subject your user to interrogation by a
computer. I come from the school of thought that says humans tell computers what to
do, not the other way around. In addition to being pro-human, that policy has many practical benefits.
For example, isn't it annoying that everything time you edit the code and
re-run it, you have to answer the same damn questions. A different approach
is to take those instructions directly from the command line. Python
ships with a module called argparse
that would work well for a script
like this, but you can skip that if you want and just use sys.argv
directly.
You probably should do some input validation, but I'll leave that
for you to pursue or for some other reviewer to comment on. Here's
the code with those suggested changes.
from string import digits, ascii_letters, punctuation
from random import choice
from sys import argv
if len(argv) == 3:
chartype = args[1]
n = int(args[2])
else:
prompt = 'Password characters:\n (D)igits\n (L)etters\n (S)ymbols\n (A)ll\n'
chartype = input(prompt)
n = int(input("Length? "))
chartype = chartype.lower()[0]
characters = {
'd': digits,
'l': ascii_letters,
's': punctuation,
'a': digits + ascii_letters + punctuation,
}
pw = ''.join(choice(characters[chartype]) for i in range(n))
print(pw)