3
\$\begingroup\$

I wrote a simple program to print out any vowels found in a string, as well as the amount of times that vowel occurs.

Here is my code:

#include <iostream>
#include <string>
#include <map>

void findVowels(std::string& theString, std::map<char,int>& map);
bool vowelCount(char& temp);
std::ostream& operator<<(std::ostream& out, const std::map<char, int>& map);

int main()
{
    bool keepGoing = true;
    while (keepGoing)
    {
        std::cout << "Enter a string to see how many times vowels were used.\n";
        std::string userInput;
        std::getline(std::cin, userInput);

        std::map<char, int> theMap;
        findVowels(userInput, theMap);

        std::cout << theMap;
        std::cout << "\nWould you like to keep going? Please type Y to continue or N to exit.\n";

        char nextAction;
        std::cin >> nextAction;
        if (nextAction == 'N' || nextAction == 'n')
            keepGoing = false;
        else if (nextAction != 'Y' && nextAction != 'y')
            std::cout << "Incorrect input. Defaulting to yes.\n";
        std::cin.ignore();
    }
    std::cout << std::endl;
    system("pause");
    return 0;
}

void findVowels(std::string& theString, std::map<char, int>& theMap)
{
    int stringSize = static_cast<int>(theString.size());
    for (int i = 0; i < stringSize; i++)
    {
        if (vowelCount(theString[i]))
            theMap[theString[i]]++;
    }
}

bool vowelCount(char& temp)
{
    temp = toupper(temp);
    return (temp == 'A' || temp == 'E' || temp == 'I' || temp == 'O' || temp == 'U');
}

std::ostream& operator<<(std::ostream& out, const std::map<char, int>& theMap)
{
    std::map<char, int>::const_iterator iter = theMap.cbegin();
    std::map<char, int>::const_iterator iterEnd = theMap.cend();
    while (iter != iterEnd)
    {
        out << iter->first << " has " << iter->second << " number of occurences.\n";
        iter++;
    }

    return out;
}
\$\endgroup\$
5
\$\begingroup\$

I agree mostly with @Yuri, but I feel that regex is an overkill here. Besides, as they say: "Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems".

Check if a letter is a vowel

I'll begin with this function since it's at the core of your program:

// your code
bool vowelCount(char& temp) 
{
    temp = toupper(temp);
    return (temp == 'A' || temp == 'E' || temp == 'I' || temp == 'O' || temp == 'U');
}

I didn't understand at first why you would pass the argument as a non-const reference. I realized only after that you relied on the inner toupper to store vowels in theMap as capital letters. That's a bad idea! One function should have one purpose and its purpose should be expressed clearly. In this case it assesses if the letter is a vowel, period.

So use a const reference instead or, even better, a copy of the character you take as an argument. A char is 1 byte, so taking it by reference or pointer never is an optimization and can even be a pessimization.

The other thing I find sub-optimal in your code is the way you test if temp is a vowel. Firstly because there are better, faster ways to find a value in a range, like binary_search, which is well known as well as available in the standard library (in <algorithm>). Secondly because the vowels' list isn't the same in all languages. For instance, in french, you would need to count 'y', 'à', 'â', 'é', 'è', 'ê', 'î', 'ô', 'ù', 'û', as well. That's why it might be more flexible and readable to keep the list separated from your code.

So, I would have something like:

bool is_vowel(char c, const std::string& sorted_vowel_list) {
    return std::binary_search(sorted_vowel_list.begin(),
                              sorted_vowel_list.end(),
                              std::toupper(c));
}

A Vowels' map

I'd then count the vowels into a map almost the way you do; I'd only use a range-based loop:

for (auto letter : input_string)
    if (is_vowel(letter))
        ++vowel_map[std::toupper(letter)];

If C++17 is available to you, you can then use "structured bindings" to read the map without referring to iterators:

for (auto [vowel, count] : vowel_map)
    std::cout << "Vowel: " << vowel << " has " << count << " occurrences\n";
\$\endgroup\$
2
\$\begingroup\$

Unless you did it specifically to showcase the interface you can move your functions before main to avoid having to forward declare them.

Try to use prefix over postfix when possible.

system("pause") is not platform independent.

return 0 at the end of main is not necessary unless you depend on the exit code somewhere else.

You should not omit braces as that can lead to hard to find bugs.

Not quite following your casting in findVowels. You still end up getting conversion warnings (-Wsign-conversion).
The conversion in vowelCount also causes a warning (-Wconversion).

I think your overloading of operator<< is a bit over the top here seeing as you only call it on your map once. A simple for range loop would probably be enough here.

All in all it seems a bit much work for such a small task. Have you thought about using regex to solve this?
A very simple regex-based approach could look like this:

#include <string>
#include <regex>
#include <map>
#include <iostream>
#include <locale>

int main() {
    std::string line;
    std::cin >> line;
    std::regex rx{"([aeiou])", std::regex_constants::icase};
    auto line_begin = std::sregex_iterator(line.begin(), line.end(), rx);
    auto line_end = std::sregex_iterator();

    std::map<char, int> vowel_count;
    for (std::sregex_iterator it = line_begin; it != line_end; ++it) {
        std::smatch match = *it;
        ++vowel_count[std::toupper(match.str().at(0))];
    }

    for (auto const& pair : vowel_count) {
        std::cout << pair.first << ": " << pair.second << "\n";
    }
}

We simply match all vowels of either case then print how often they were matched respectively. Note that printing the map is really not a lot of trouble using the for range notation.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the response. I actually have not learned anything about regular expressions so I will add that on my to do list. Would you say it's decent for a non-regex implementation? Also, in regards to casting in my findvowels function, I was taught that because a std::string's size() returns a size_t and not an int, it was safer to cast it to an int and then use it. Was this incorrect? \$\endgroup\$ – codewriter Jun 15 '18 at 8:50
  • 1
    \$\begingroup\$ @codewriter (1) I think the code itself isn't bad, you certainly avoided many mistakes beginners tend to make. (2) There is an implicit conversion from int to the string size_type when you access it. You could just use a matching size_type for the loop. Also the compiler will most likely optimize the stringSize so that can be omitted. \$\endgroup\$ – yuri Jun 15 '18 at 8:59
  • \$\begingroup\$ @codewriter You might want to wait 3-4 days before accepting an answer. Other people might have a better one and if there is already an accepted answer it might discourage others. \$\endgroup\$ – yuri Jun 15 '18 at 9:57

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.