7
\$\begingroup\$

After asking my first question got a nice review on that. So, this is my solution for a different question checking edge cases and incorporating ideas given in that review.

Question: Write a program to count total number of vowel and consonant characters present in an user entered sentence?

Expected Output: If user inputs,

  • 1CHELSEA. 1FOOTBALL. 1CLUB. -> Vowel: 7, Consonant: 12

  • a1?3e5i6!@o7^&*8)u -> Vowel: 5, Consonant: 0

  • q1?32536!@47^&*8)p -> Vowel: 0, Consonant: 2

My solution works for all expected cases and from previous review suggestion I tried docstring, doctest, etc. So from this review I would like to know whether I am using those correctly or not, and if there's easier and better way to do this.

My solution:

from sys import exit


def counter(sentence):
    """Removes whitespaces, punctuation and capitalize every words.

    >>> counter("1CHELSEA. 1FOOTBALL. 1CLUB.")
    Vowel: 7, Consonant: 12

    >>> counter("a1?3e5i6!@o7^&*8)u")
    Vowel: 5, Consonant: 0

    >>> counter("q1?32536!@47^&*8)p")
    Vowel: 0, Consonant: 2

    """
    count_sentence = "".join(ch for ch in sentence if ch.isalpha()).upper()  # removes punctuation and numbers

    def find_total(count):
        """Check if character is present in vowel_list if present ++ in total"""
        vowel_list = ["A", "E", "I", "O", "U"]
        total = 0
        for character in count:
            if character in vowel_list:
                total += 1
        return total

    find_total(count_sentence)
    vowel_total = find_total(count_sentence)
    consonant_total = len(count_sentence) - vowel_total
    print("Vowel: {0}, Consonant: {1}".format(vowel_total, consonant_total))
    return


def main():
    """Checks for blank input."""
    entered_sentence = input("Please enter a sentence?\n>")
    if entered_sentence.strip() == "":
        exit("Program Terminated. (No input)")
    else:
        counter(entered_sentence)
    return


if __name__ == '__main__':
    import doctest
    doctest.testmod(main())

Lastly, please help me on returning from a function and separating printing part not been able to understand that.

\$\endgroup\$
10
\$\begingroup\$

Some thoughts:

choose good names

counter is IMO a bit "generic". Perhaps something more expressive like count_vowels_consonants?

Moreover, the inner function's parameter is named count, but it's actually not some count (number) but a text string.

Furthermore you're using the term "sentence", but you're not actually requiring sentences, but rather character sequences / strings.

don't call the function twice

find_total(count_sentence)
vowel_total = find_total(count_sentence)

Here you call find_total, discard its result (return value) and then call it again, this time assigning the result to the local variable.

separate calculation and output

Instead of

print("Vowel: {0}, Consonant: {1}".format(vowel_total, consonant_total))
return

leave out the print and return the values in a tuple

return (vowel_total, consonant_total)

On the caller side, you can then either use the returned tuple as it is

t = count ("foo bar")

or for example use tuple unpacking to obtain the individual counts

vowels, consonants = count ("foo bar")

only iterate once over the characters

Instead of first going one time over the characters to filter out non alphabetic characters and adjust the case of the alphabetic ones and then a second time to count, do it in one loop:

def count_vowels_consonants(text):
  vowels_list = ['A', 'E', 'I', 'O', 'U']
  consonants = 0
  vowels = 0
  for character in text:
    if character.isalpha():
      if character.upper() in vowels_list:
        vowels += 1
      else:
        consonants += 1
  return (vowels, consonants)

test corner cases

Does your function work well with an empty string? A string with only vowels or consonants? A string with only punctuation?

\$\endgroup\$
  • 3
    \$\begingroup\$ Good answer! I'd only added: use VOWELS = frozenset("AEIOU") "constant" at module level, so there will be no need to construct vowels_list in the function itself. Usage is the same as before: character.upper() in VOWELS \$\endgroup\$ – Roman Susi Oct 8 '17 at 13:48
  • \$\begingroup\$ @Daniel Jour The concept I am not being able to grasp is how to use those return values. For me code reviewing has been the best way of learning so far, learned so much things in two days. So thanks ++ \$\endgroup\$ – Ram Chandra Giri Oct 8 '17 at 14:07
  • \$\begingroup\$ @RomanSusi Haven't gotten into Sets yet, so refraining from that . Once I learned that will implement frozenset. \$\endgroup\$ – Ram Chandra Giri Oct 8 '17 at 14:09
  • \$\begingroup\$ @Ram I've added a bit on usage of the returned value, does that help? \$\endgroup\$ – Daniel Jour Oct 8 '17 at 14:23
  • 1
    \$\begingroup\$ @DanielJour Finally able to understand unpacking return values. Tested with your changes works great, lot easier to understand. Again thanks. \$\endgroup\$ – Ram Chandra Giri Oct 8 '17 at 14:36
4
\$\begingroup\$

I guess you learn fast. Your code is fine. I just have 2 small comments:

  • You used return statement in counter(). It is useless in your context because in Python even when you do not specify a return value, a function returns None anyway. So you can simply omit that statement.
  • The second small issue I see, IMHO, is rather a design-related one. I mean you are using the notion of nested functions while they are unnecessary in your context. The best use of nested functions is to devise decorators.
\$\endgroup\$
  • \$\begingroup\$ Sir, I put return statement there to learn separating print from that function and return value but not been able to grasp that concept. And for that nested function it was very long without that so I thought to nest function. Sir, great article bookmarked it. Will learn about decorator later. Thank you. ++ \$\endgroup\$ – Ram Chandra Giri Oct 8 '17 at 11:18
3
\$\begingroup\$

Have you encountered f-string notation yet? You just put a f before the quotation marks in your print function, then pave the variables inside curly brackets in the paces you want them in your printed string. So:

print("Vowel: {0}, Consonant: {1}".format(vowel_total, consonant_total))

Becomes:

print(f"Vowel: {vowel_total}, Consonant: {consonant_total}")

Which (to me at least) is a lot more readable.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome :) They're fairly new, but much easier to look at, and I believe much faster than the other methods when you run it. \$\endgroup\$ – toonarmycaptain Oct 9 '17 at 11:16

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.