5
\$\begingroup\$

I am coming to a problem where I am writing a function that accepts a pointer to a C-string as its argument. The function should count the number of vowels appearing in the string and return that number. Then, I am writing another function that accepts a pointer to a C-string as its argument. This function should count the number of consonants appearing in the string and return that number.

Am I violating any of the following restrictions?

  • No global variables
  • No labels or go-to statements
  • No infinite loops, examples include: for(;;), while(1), while(true), do{//code}while(1);
  • No break statements to exit loops

Here is my code:

#include<iostream>

using namespace std;

//Function prototypes
int Vowel_count( char *str);

int Consonant_count(char *str);

 int main()

    {

     char String[30];

     char choice;

     int Nvowels,Nconsonant;

     //Input a string

     cout<<"Enter a string:";

     cin.getline(String ,30);

     do

     {

     //Displaying Menu

     cout<<"\nMENU"<<endl;

     cout<<" (A) Count the number of vowels in the string"<<endl;

     cout<<" (B) Count the number of Consonants in the string"<<endl;

     cout<<" (C) Count both the vowels and consonants in the string"<<endl;

     cout<<" (D) Enter another string"<<endl;

     cout<<" (E) Exit program"<<endl;

     //Inputting choice

      cout<<"Enter choice";

     cin>>choice;

     switch(choice)

     {

     case 'A':
     //Function call to get number of Vowels

               Nvowels=Vowel_count(String);  
                cout<<"Number of vowels is:"<<Nvowels<<endl;

              break;

     case 'B': 
     //Function call to get number of consonants

                Nconsonant=Consonant_count(String);

                //Outputting number of Consonants

               cout<<"Number of Consonants is:"<<Nconsonant<<endl;

                 break;

     case 'C': 
     //Function call to get number of Vowels

                Nvowels=Vowel_count(String);

                //Function call to get number of consonants

                Nconsonant=Consonant_count(String);

              //Outputting Both Vowels and Consonants

              cout<<"Number of vowels is:"<<Nvowels<<endl;

                   cout<<"Number of Consonants is:"<<Nconsonant<<endl;

                 break;

     case 'D': //Inputting another string

                 cout<<"Enter another String:";

                 fflush(stdin);

                 cin.getline(String ,30);

                 break;

     case 'E': exit(0);

                  break;  

     }//End of Switch

     }while(choice !='E');//End Do while

    //Pause system for a while

     system("pause");

}//End main

//Function Definitions

int Vowel_count( char *str)

{  

   int count=0;//Local variable

   //Checks for all characters in C-string

   while(*str!='\0')

   {

    if(*str=='a'||*str=='e'||*str=='i'||*str=='o'||*str=='u')

         count++;

         str++;

   }//End While

//Returns count value to function call

   return count;

}//End of Vowel_count

int Consonant_count(char *str)

{

int count=0;

    //Checks for all characters in C-string

     while(*str!='\0')

     { //Checks for consonants

     if(*str!='a'&&*str!='e'&&*str!='i'&&*str!='o'&&*str!='u')

          count++;

        str++;

     }//End While

     //Returns count value to function call

     return count;

}
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

I see a number of things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers).

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, for pause you might use this:

void pause()
{
    std::string;
    std::cout << "Press enter to continue...\n";
    std::cin >> foo;
}

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use string concatenation

The menu includes these lines:

cout<<"\nMENU"<<endl;
cout<<" (A) Count the number of vowels in the string"<<endl;
cout<<" (B) Count the number of Consonants in the string"<<endl;
cout<<" (C) Count both the vowels and consonants in the string"<<endl;
cout<<" (D) Enter another string"<<endl;
cout<<" (E) Exit program"<<endl;

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

std::cout << 
    "\nMENU\n"
    " (A) Count the number of vowels in the string\n"
    " (B) Count the number of Consonants in the string\n"
    " (C) Count both the vowels and consonants in the string\n"
    " (D) Enter another string\n"
    " (E) Exit program\n"
    "Enter choice\n";

This reduces the entire menu to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Check your if statements for proper braces

I suspect that the if statement in the Vowel_count() routine is not what you intended to write. It's written like this:

if(*str=='a'||*str=='e'||*str=='i'||*str=='o'||*str=='u')
    count++;
    str++;

but because there are no braces, what's actually being executed is this:

if(*str=='a'||*str=='e'||*str=='i'||*str=='o'||*str=='u') {
    count++;
}
str++;

That is the correct operation, but it's confusing to the reader. I recommend using the braces always to avoid ambiguity or confusion.

Use const where possible

The Vowel_count and Consonant_count functions do not (and should not) alter the passed char *, so that parameter should be passed as const.

Fix the bug

If the user enters the string "My dog has fleas.", the program will report 4 vowels and 13 consonants, but that's not actually correct. The 'y' in "My" is a vowel in this usage and the spaces and period are not consonants.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks @Edward for the helpful tips. I will def. fix it. But, does it apply to the restrictions I have above? Or Program free from those restrictions? \$\endgroup\$
    – CApps
    Feb 18, 2019 at 22:57
  • \$\begingroup\$ Yes, it appears to comply with the stated restrictions. Also, I should mention that those restrictions are generally good programming practices, so keep them in mind for future programs as well. \$\endgroup\$
    – Edward
    Feb 18, 2019 at 23:26
  • \$\begingroup\$ Determining whether y is acting as a consonant or as a vowel adds complexity, but perhaps it's an interesting problem for a beginner... \$\endgroup\$ Feb 19, 2019 at 10:18
3
\$\begingroup\$
    char String[30];
    char choice;
    int Nvowels,Nconsonant;

Don't define a variable before it is needed. Doing so increases the cognitive load on readers that trace through your code and keep mental records of every variable, their possible values, and whether they've even been set. This artifact from older languages also encourages lazy reuse of variables beyond their original intent. Define variables as you need them and keep the scope of each variable as narrow as required.

Use descriptive names. The larger the scope in which a variable exists, the more important it is to give that variable a name that indicates to the reader what it represents. String \$\rightarrow\$ input_sentence. Nvowels \$\rightarrow\$ vowel_count.


    cin.getline(String ,30);

If a user inputs 30 or more characters, std::istream::getline will set the failbit. If end-of-file is reached before the delimiter is encountered, std::istream::getline will set the eofbit. You should check if std::cin is in a readable state and reset the state if it's not, otherwise you'll have an infinite loop when an error is encountered.


    cout<<"\nMENU"<<endl;
    cout<<" (A) Count the number of vowels in the string"<<endl;
    cout<<" (B) Count the number of Consonants in the string"<<endl;
    cout<<" (C) Count both the vowels and consonants in the string"<<endl;
    cout<<" (D) Enter another string"<<endl;
    cout<<" (E) Exit program"<<endl;

Whitespace is great for readability as they help separate operators and language constructs.

Avoid std::endl. If you need to explicitly flush the cache, then explicitly stream the manipulator std::flush and comment why a flush is necessary. If you simply need a end-of-line character, prefer '\n' or "\n".

Edward covered string concatenation via the compiler, but I'd like to also point out that C++11 introduced raw string literals.

    std::cout << R"(
MENU
 (A) Count the number of vowels in the string
 (B) Count the number of Consonants in the string
 (C) Count both the vowels and consonants in the string
 (D) Enter another string
 (E) Exit program
Enter Choice
");

    cin>>choice;

As with std::istream::getline, std::istream::operator>> needs to be checked to ensure a read was successful. If extraction failed, you should probably notify the user and attempt to recover.


    case 'A':
    //Function call to get number of Vowels
    case 'B':
    //Function call to get number of consonants
    //Outputting number of Consonants
    case 'C':
    //Function call to get number of Vowels

Keep comments crisp. Everything I need to know as far as what is being done is already there in the form of code to be compiled. Comments should be reserved for explaining why the code was written the way it was (the intent). Adding verbosity slows down understanding.

To assist with the readability, consider mapping the character inputs to enumerations. You can read up on enum serialization, but just to give you an idea of how it can help readability:

enum class Selected {
    count_vowels = 'A',
    count_consonants,
    count_both,
    reset_sentence,
    exit
};

std::istream& operator>>(std::istream& in, Selected op) {
    // exercise for the reader. 
}

int main() {
    // ...
        Selected user_choice;
        std::cout << "Enter choice: ";
        std::cin >> user_choice;

        switch (user_choice) {
        case Selected::count_vowels:
            // ...
        case Selected::count_consonants:
            // ...
        case Selected::count_both:
            // ...
        case Selected::reset_sentence:
            // ...
        case Selected::exit:
            // ...
        case default:
            // Handle user-input error (out of range, end of file)

            fflush(stdin);

Using std::fflush on an input stream is undefined behavior. If you were trying to reset the buffer of std::cin, see std::cin::ignore.


        case 'E': 
            exit(0);
            break;
        }
    } while (choice != 'E');

    system("pause");

Once the the program encounters exit(0), all of the remaining code (break; \$\rightarrow\$ while (choice != 'E') \$\rightarrow\$ system("pause");) is unreachable. If you intend to immediately return from a function (or in this case exit the program), you don't need to loop until choice is \$E\$. This is one of those times where while (true) is actually appropriate if you didn't have the restriction.

system("pause") is slow, platform-dependent, and insecure. Consider re-evaluating your development environment, from having permanent access to a console or utilizing the breakpoint of a debugger.


int Consonant_count(char *str) {
    // ...
        if (*str != 'a' && *str != 'e' && *str != 'i' && *str != 'o' && *str != 'u')
              count++;
            str++;

Prefer scoping your single-line statement blocks with braces. Adding code in a hurry

if (condition)
    statement;
  statementIntendedToBeScoped???

or commenting

if (condition)
    // statement
statementThatIsNowScopedToCondition

can lead to errors. Scoping with braces addresses this issue.

if (condition) {
    statement
}
statementDefinitelyNotScoped

if (condition) {
    // statement
}
statementDefinitelyNotScoped

if (*str != 'a' && *str != 'e' && *str != 'i' && *str != 'o' && *str != 'u')

Keep in mind the various possible values a type can represent. A character (char) can represent the set of consonants and vowels (A-Z, a-z). It can also represent digits, punctuation, control characters, whitespace, etc. The conditional in Consonant_count is returning true or false if the current character is any ASCII character (not any alphabetic character) that is not any of aeiou. Clearly control structures, punctuation, and whitespaces are not consonants.

In both of your functions, how do you handle uppercase vowel inputs?

Rather than check each individual vowel, consider a lookup technique. You could create a table (one for vowel, one for consonant) that maps each character to true or false. There is also a bit mapping alternative that builds on the lookup table approach, where you exploit the layout of the ascii table to do very fast lookups via bit shifts and masks.

bool is_vowel(char ch) {
    auto lowercase_pos = (ch | 0x20) - 96;
    if (static_cast<unsigned char>(lowercase_pos) > 31) {
       return false;
    }
    constexpr std::uint32_t vowel_mask = (1 << 1) | (1 << 5) | (1 << 9) | (1 << 15) | (1 << 21);
    return (vowel_mask & (std::uint32_t{1} << lowercase_pos)) != 0;
}

specific_count(range)
    count = 0
    for each element in the range
        if element is what we're looking for
            count = count + 1
    return count

Learn the various <algorithm>s. You'll notice that both your functions have the same structure as the pseudocode above. It turns out, this is a common algorithm in C++ (std::count_if). You should be aware of this library as you learn more about functions and specifically lambdas (objects that behave like functions and can be passed around into functions as arguments).

\$\endgroup\$
0
\$\begingroup\$

You can use standard algorithms to implement vowel or consonant count, e.g.,

int vowel_count(const std::string& s)
{
    return std::count_if(s.begin(), s.end(), [](char c) { 
        return c == 'a' || c == 'e' || c == 'i' || c == 'o' || c == 'u'; }
    );
}

Clearly, you can apply the same idea for counting consonants (or even implement that in terms of vowel counting, i.e., every character that is not a vowel is a consonant, but this requires you to trust the input to be sane).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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