11
\$\begingroup\$

I'm learning to program and I've chosen Python as my first language. I've written a function to count vowels in a string and print the number of occurrences of each of them. Please review my code and let me know if there is a better way of writing the same as I believe there is always room for improvement.

vowels = 'aeiou'

def vowel_count(txt):
    for vowel in vowels:
        if vowel in txt:
            print vowel, txt.count(vowel)

txt_input = raw_input('Enter text: ').lower()

vowel_count(txt_input)
\$\endgroup\$
6
\$\begingroup\$

For a novice, your code is good. The variable names are good (though txt should be text, if I was pedantic....), and the code is logical, and well structured.

There are ways to improve the performance a bit. I suspect that looping through the text 10 times (once for the if vowel in txt, once for the txt.count(vowel), and repeated for each vowel...) is excessive. In fact, you could reduce this to just one loop through the text, but the overall saving will be quite small (for small text sizes).

My real concern is if there are no vowels, then your program produces no output.

I would print the count of the vowels even if the count is 0. This shows that the code is working, and checking things. This would remove the need for the inner if-conditions too:

def vowel_count(txt):
    for vowel in vowels:
        print vowel, txt.count(vowel)

(and remove one scan of the text as well).

\$\endgroup\$
  • \$\begingroup\$ Now I can see why I don't need the if-statement at all. I am a little pedantic too :) You're right there should be some output even if there're no vowels in the input. Thank you very much. \$\endgroup\$ – user61142 Dec 15 '14 at 0:36
  • \$\begingroup\$ I almost missed the gist of "...looping through the text 10 times...", need to be more attentive. \$\endgroup\$ – user61142 Dec 15 '14 at 11:02
6
\$\begingroup\$

A better One way would be to loop over the txt itself and use collections.Counter to get the count of each vowel. This way you'll loop over txt only once:

from collections import Counter

def vowel_count(txt):
    c = Counter(c for c in txt if c in vowels)
    for v in vowels:
        print v, c[v]

A Counter object returns 0 for missing keys so no need to worry about checking for key first.

Though it's worth mentioning that as we are only getting the count of 5 items here the str.count method will outperform Counter or any other method that involves looping in Python:

>>> txt = ''.join(random.choice(ascii_lowercase) for _ in xrange(10**6))
>>> %timeit vowel_count(txt) #rolfl's version
100 loops, best of 3: 5.12 ms per loop
>>> %timeit vowel_count_with_counter(txt) #using `Counter`
10 loops, best of 3: 47.1 ms per loop
>>> %timeit vowel_count_with_dd(txt) #using `collections.defaultdict`
10 loops, best of 3: 33 ms per loop
>>> %timeit vowel_count_with_dict(txt) #using normal `dict`
10 loops, best of 3: 38.1 ms per loop
\$\endgroup\$
5
\$\begingroup\$

The check if vowel is in the text is unnecessary: when you count the occurrences, you will know if count is zero or not, and that way you will save some unnecessary processing. Like this:

def vowel_count(txt):
    for vowel in vowels:
        count = txt.count(vowel)
        if count:
            print vowel, count

Also, since the primary purpose of the method is to print something, I would add a print_ prefix to it.

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm laughing at myself as you made me realize why the check is not needed. Thank you \$\endgroup\$ – user61142 Dec 15 '14 at 0:42

Your Answer

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