Skip to main content
4 of 4
added 5 characters in body
Snowhawk
  • 6.8k
  • 1
  • 19
  • 37
    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).

Snowhawk
  • 6.8k
  • 1
  • 19
  • 37