17

I'm pretty new to Python and programming in general, and I was wondering if it is a good programming practice to write long statements with many logic operators - for example, in a for loop.

For example, here's a function I made that gets all the vowels from a word and returns a list containing those vowels.

def getVowels(word):
    vowel_list = []
    index = 0
    for i in word:
        if i == "a" or i == "e" or i == "i" or i == "o" or i == "u" or i == "A" or i == "E" or i == "I" or i == "O" or i == "U":
            vowel_list.append(word[index])
        index += 1
    return vowel_list

As you can see, the if statement has gotten very long. Is it considered good programming? If it's not, is there a better way to code this function?

2
  • 1
    You should use enumerate instead of a counter.
    – agf
    Apr 7, 2012 at 13:59
  • This is a good question. Be sure to accept an answer in the next couple of days.
    – Marcin
    Apr 7, 2012 at 14:10

5 Answers 5

34

No it is not considered good practice, there are always better ways :D

if i.upper() in "AEIOU"

Here is a much shorter version of your function using list comprehensions:

def get_vowels(word):
    vowels = "AEIOU"
    return [c for c in word if c.upper() in vowels]
5
  • Thank you. I didn't know i can use "in" for strings too , i thought i can only use it for lists. But as strings behave almost the same as lists, i should've known that .d Thank you everyone.
    – stensootla
    Apr 7, 2012 at 14:02
  • 1
    Yeah there are a lot of shortcuts :D
    – jamylak
    Apr 7, 2012 at 14:03
  • 3
    @geekkid The ability to compress your code down to this answer is pretty much what python is all about - it really enables concise, non-repetitive coding.
    – Marcin
    Apr 7, 2012 at 14:09
  • 6
    Just as a minor nitpick, Python tends to use underscored_names like get_vowels.
    – Peter C
    Apr 7, 2012 at 19:14
  • Also str.upper() isn't polite to use on Unicode (what is 'и'.upper()?) It would be better (and more concise!) to test upper and lowercase explicitly: if i in "AEIOUaeiou". Apr 10, 2012 at 16:10
4

Would probably be better to use sets:

VOWELS = set('aeiouAUIOU')

def get_vowels(word):
    return [c for c in word if c in VOWELS]

or, more geeky:

def get_vowels(word):
    return filter(VOWELS.__contains__, word)

(But the first approach is most readable and as such is more pythonic. Also, the second function will return generator, not a list in Python 3.)

EDIT performance comparison of c in list vs c in set:

import timeit

VOWELS = 'aeiouAEIOU'
VOWSET = set(VOWELS)
SAMPLE = 'asflasrjoperugASDFAROUAoarfpeoriugargagadropgue'

def get_vowels(word, vowels):
    return [c for c in word if c in vowels]

print timeit.timeit('get_vowels(SAMPLE, VOWELS)', 
                    'from __main__ import VOWELS, SAMPLE, get_vowels') 
# ^ prints 10.0739870071
print timeit.timeit('get_vowels(SAMPLE, VOWSET)', 
                    'from __main__ import VOWSET, SAMPLE, get_vowels') 
# ^ prints 9.43965697289
5
  • 1
    Your last function doesn't produce the vowels in the word, it just says True or False
    – jamylak
    Apr 7, 2012 at 15:07
  • @jamylak oh shame on me. Thanks, removing.
    – bereal
    Apr 7, 2012 at 15:09
  • Is using sets make it faster? Because VOWELS = 'aeiouAUIOU' also works.
    – Akavall
    Apr 7, 2012 at 15:18
  • @Akavall yes, it's marginally faster, updated with timeit output. For sequences longer than aeiouAEIOU difference will be more significant.
    – bereal
    Apr 7, 2012 at 15:37
  • 4
    Set contains is average-case constant time, list is linear time. The number of characters is tiny here so it makes almost no difference.
    – Ali Afshar
    Apr 7, 2012 at 16:17
1

The if is the same as:

if i in "aeiouAEIOU"

Basically you're checking for member in a set.

1
  • @agf They're equivalent if we assume word is a string Apr 7, 2012 at 13:57
1

I think long statements are harder to understand than short ones. There is almost always a way to do the same thing with shorter statements. In your case, you can simplify the if like this:

def getVowels(word):
    vowel_list = []
    for i in word:
        if i in "aeiouAEIOU":
            vowel_list.append(i)
    return vowel_list

since Python allows you to use the "in" operator to search for one string inside another.

But Python also allows list comprehensions, which simplify loops:

def getVowels(word):
    return [i for i in word if i in "aeiouAEIOU"]
1

Like:

set('aeiouAUIOU') & set(word)
2
  • Ooh, pretty... Except that scrambles the letters, and doesn't preserve duplicates. Apr 7, 2012 at 23:38
  • Yes, and yes, but too pretty to miss out.
    – Ali Afshar
    Apr 8, 2012 at 1:12

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.