Determining the number of vowels in each word from a file

Question

I'm 5 weeks into my first C++ class. I'm curious about how this code could be improved, more streamlined or simply things that I could/should have done better.

The project was:

Read from a .txt file that has different words on separate lines. Determine how many vowels each word has, and print each word AND vowel count to the console. You must write and use a function outside of main().

Here's the code I wrote:

#include <iostream>
#include <string>
#include <fstream>
using namespace std;
//prototypes
void readFile();
void printWords(string);
int main()
{
 readFile();
 return 0;
}//end main
void readFile()
{
 ifstream inFile;
 string words;
 inFile.open("words.txt");
 while(!inFile.eof())
 {
 inFile>>words;
 printWords(words);
 }//end while loop
 inFile.close();
}//end readFile function
void printWords(string str)
{
 char a = 'a', e = 'e', z = 'i', o ='o', u ='u';
 int num_A = 0, num_E = 0, num_I = 0, num_O = 0, num_U = 0;
 cout<< str <<endl;
 for(int i = 0; i < str.length(); ++i)
 {
 if(str.at(i) == a)
 {
 num_A++;
 }
 if(str.at(i) == e)
 {
 num_E++;
 }
 if(str.at(i) == z)
 {
 num_I++;
 }
 if(str.at(i) == o)
 {
 num_O++;
 }
 if(str.at(i) == u)
 {
 num_U++;
 }
 }//end for loop
 cout<< "a: " << num_A <<endl;
 cout<< "e: " << num_E <<endl;
 cout<< "i: " << num_I <<endl;
 cout<< "o: " << num_O <<endl;
 cout<< "u: " << num_U << "\n" <<endl;
}//end printWord function

I feel like there should be a smoother function to extract, count and return the vowels. Then a function to print. Also, I tried to use a switch statement because there were so many if statements, but I couldn't get it to work.

• Neil Kirk: while(!inFile.eof()) is bad, see stackoverflow.com/questions/5605125/… instead use while (inFile >> words). Pass string to printWords as const reference.
• Keugyeol: Several if-statements in the for-loop can be implemented with switch(str.at(i)). You can remove char variable declarations and use literal values for each cases e.g. case 'a':, case 'e': ...
• Paweł Stawarz: Determine how many vowels each word has, and print each word AND vowel count to the console. means "vowels in total", or "each vowel separatelly"? If the first - you just need 1 counter.
• Greg Hewgill: z = 'i' That's not confusing at all. :)
• molbdnilo: Don't use plural forms ("words") for singular entities and don't name a function "printWords" if it doesn't print words.

Answer 1

Don't do this:

using namespace std;

See: Why is "using namespace std;" considered bad practice?

Pass by value?

void printWords(string);

You are not planning on modifying so pass by const reference in preference. This will prevent an extra copy. This also tells the compiler you don't want to modify the word and thus the compiler can potentially do optimizations as it does not need to worry about mutations.

Main is special.
You do not need an explicit return. If non is defined then an implicit return 0; is panted for you. In the case where there is no chance of failure I prefer not to use an explicit return.

int main()
{
 readFile();
 return 0;
}//end main

May as well create and open file in a single statement.

 ifstream inFile;
 string words;
 inFile.open("words.txt");
 // I would have used:
 std::ifstream inFile("words.txt");
 std::string words;

This is always wrong:

 while(!inFile.eof())

see: Why is iostream::eof inside a loop condition considered wrong?

It should be re-written as:

 while(inFile >> word)
 {
 // STUFF
 }

But you can go further. You can use iterators and loops to achieve the same affect.

 for(std::istream_iterator<std::string> loop(inFile); loop != std::istream_iterator<std::string>(); ++loop)
 {
 // STUFF
 }

Now that you can see a loop using iterators you can look at using some of the standard algorithms.

 std::for_each(std::istream_iterator<std::string>(inFile),
 std::istream_iterator<std::string>(),
 [](std::string const& word){ /* STUFF */ }
 );

Don't manually close a file:

 inFile.close();

see: My C++ code involving an fstream failed review

As noted above:

void printWords(std::string cosnt& str)
{

If variables do not change then mark them constant.

 char a = 'a', e = 'e', z = 'i', o ='o', u ='u';

I would have used an array here:

 int num_A = 0, num_E = 0, num_I = 0, num_O = 0, num_U = 0;

Prefer not to use std::endl unless you want to force a flush (you usually don't as it is very costly). Prefer to use "\n" this has the same effect but with no flush.

 cout<< str <<endl;

If you know your access is in bounds then prefer to use operator[] it does no checking and is thus faster. Prefer .at() if you want to force the check.

 if(str.at(i) == a)

Saying all that I would have use a standard function to count vowels.

std::count_if(std::begin(word), std::end(word), [](char l){return isVowel(l);});
bool isVowel(unsigned char l)
{
 static char vowel[] = 
 { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1, // aeio
 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // u
 0, 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1, // AEIO
 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // U
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
 return vowel[l];
}

My solution would be:

#include <string>
#include <fstream>
#include <iostream>

WordInfo class

class WordInfo
{
 std::string word;
 int count;
 public:
 WordInfo(std::string const& word)
 : word(word)
 , count(0)
 {
 count = std::count_if(std::begin(word), std::end(word),
 [](char l) {return isVowel(l);});
 }
 friend std::ostream& operator<<(std::ostream& str, WordInfo const& data)
 {
 return str << data.word << " " << data.count;
 }
};

main.cpp

int main(int argc, char* argv[])
{
 if (argc < 2)
 {
 std::cerr << "Error: Wrong usage\n";
 return -1;
 }
 std::ifstream infile(argv[1]);
 std::copy(std::istream_iterator<std::string>(infile),
 std::istream_iterator<std::string>(),
 std::ostream_iterator<WordInfo>(std::cout, "\n")
 );
}

Note: The std::copy above acts like this:

 std::string word;
 while(infile >> word)
 {
 std::cout << WordInfo(word) << "\n";
 }

Answer 2

Here is what I would have done. You can see many differences in amount of code necessary, which comes just from experience. The changes made may seem large but are minor and just in efficiency of writing short code.

#include <fstream>
#include <iostream>
using namespace std;
void printWords(string str){
 char vowels[5] = {'a', 'e', 'i', 'o', 'u'};
 int counter[5] = {0};
 cout << str << endl;
 for(int a = 0; a < str.length(); a++)
 {
 for(int b = 0; b < 5; b++)
 {
 if(str.at(a) == vowels[b])
 {
 counter[b]++;
 break;
 }
 }
 }
 for(int a = 0; a < 5; a++)
 cout << vowels[a] << ": " << counter[a] << endl;
}
int main(){
 ifstream inFile("words.txt");
 string words;
 while(inFile >> words)
 {
 printWords(words);
 }
 inFile.close();
 return 0;
}

The changes made are mostly in the printWords function, including addition of for loops and arrays instead of having many different variables. Keeping variables simple allows easier integration into for loops. If anything doesn't make sense, look it up and see if you can figure out what it does.

• Loki Astari: This answer needs its own review.