# Simple function to generate a list of letters

I would like get some suggestions for improvement of this function, and alert me about any dodgy practices that you find. I have been trying to learn python for a short time now and I would like to get some pointers on writing more pythonic code before I develop bad habits.

from string import ascii_uppercase, ascii_lowercase
from random import choice

def get_letters(case, n, random = False):
"""
A function to generate a list of letters

PARAMETERS
----------------------
case (str) - type of case, 'u' for uppercase, 'l' for lowercase
n (int) - number of letters to return
random (bool) - specify if letters should be in random order

RETURNS
----------------------
if case is 'u', returns a list of uppercase letters
if case is 'l', returns a list of lowercase letters
if random = True a list of random letters will be returned (default is false)
"""

case = case.lower()

if random:
if case == 'u':
letters = [choice(ascii_uppercase) for i in range(n)]
else:
if case == 'l':
letters= [choice(ascii_lowercase) for i in range(n)]
else:
if case == 'u':
letters = [ascii_uppercase[i] for i in range(n)]
else:
if case == 'l':
letters= [ascii_lowercase[i] for i in range(n)]

return letters

• you might want to wait a bit (24h for instance) before accepting my answer: other users might have better suggestions! (but might not take the time to write them down if they see that an answer has already been accepted) – oliverpool Mar 2 '16 at 9:15
• thanks for the tip @oliverpool - also, thanks for the pointer to yield. I had not heard of it before. I did some googleing and found jeffknupp.com/blog/2013/04/07/… as well. Very helpful. – John Mar 2 '16 at 9:17
• How do you intend to call this function, and what would you do with the result? – 200_success Mar 2 '16 at 23:20

• def f(arg = False) -> def f(arg=False) (PEP8)
• case = case.lower() Usually it's not a good idea to reuse variables: new values, new variable names.
• else + indent + if = elif
• Use (true_value if condition else false_value) for short conditional expressions.
• ascii_uppercase[i] for i in range(n). This will fail if n is greater than the size of letters.
• [choice(alphabet) for i in range(n)]. This repeats characters, is that ok? It seems more fitting to use random.sample(alphabet, n), which does not repeat elements.
• case: It's not idiomatic to send strings this way. Use a boolean argument instead (as @oliverpool proposed).

I'd write:

from random import sample
from string import ascii_uppercase, ascii_lowercase

def get_letters(n, random=False, uppercase=False):
"""Return n letters of the alphabet."""
letters = (ascii_uppercase if uppercase else ascii_lowercase)
return (sample(letters, n) if random else list(letters[:n]))

• sample seems a good idea, but it changes slightly the behavior of the OP since the returned elements are unique (sampling without replacement) [btw the list(...) is not needed]. List slicing is also much nicer than my proposition (and doesn't fail in case of index "overflow"). – oliverpool Mar 3 '16 at 6:09
• @oliverpool: yes, it was intentional, I'll add a note. – tokland Mar 3 '16 at 8:27
• The conversion to a list can be postponed. I suggest writing list(letters[:n]) instead of letters = list(…). – 200_success Mar 3 '16 at 8:45
• @200_success: you are right, I thought sample needed a list, updating. – tokland Mar 3 '16 at 8:47

First of all, I think your variable names are quite good and it is nice that you use a docstring.

## Exception handling and corner cases

You should check the arguments and raise an exception in case the case is neither u nor l (calling your code with w crashes...).

And if your call your function with a greater number than there are letters in the alphabet, how should your function behave? Adding a doctest for such cases seems a good idea!

## Suggestions

Instead of a letter, you could use a boolean and rename it to uppercase.

You can see that there is a lot of similarities between your branches, you could refactor them like this:

if uppercase:
alphabet = ascii_uppercase
else:
alphabet = ascii_lowercase


And then use alphabet:

if random:
letters = [choice(alphabet) for i in range(n)]
else:
letters = [alphabet[i] for i in range(n)]


And if you want to go further in python, instead of returning a list you can make an generator (which returns an iterator). The following should work fine for python 3:

if random:
yield from (choice(alphabet) for i in range(n))
else:
yield from (alphabet[i] for i in range(n))


And if the caller need a list, he can do list(get_letters(...))