3
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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) \$\endgroup\$ – oliverpool Mar 2 '16 at 9:15
  • \$\begingroup\$ 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. \$\endgroup\$ – John Mar 2 '16 at 9:17
  • \$\begingroup\$ How do you intend to call this function, and what would you do with the result? \$\endgroup\$ – 200_success Mar 2 '16 at 23:20
3
\$\begingroup\$

Some notes on your code:

  • 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]))
\$\endgroup\$
  • \$\begingroup\$ 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"). \$\endgroup\$ – oliverpool Mar 3 '16 at 6:09
  • \$\begingroup\$ @oliverpool: yes, it was intentional, I'll add a note. \$\endgroup\$ – tokland Mar 3 '16 at 8:27
  • \$\begingroup\$ The conversion to a list can be postponed. I suggest writing list(letters[:n]) instead of letters = list(…). \$\endgroup\$ – 200_success Mar 3 '16 at 8:45
  • \$\begingroup\$ @200_success: you are right, I thought sample needed a list, updating. \$\endgroup\$ – tokland Mar 3 '16 at 8:47
2
\$\begingroup\$

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(...))

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.