2
\$\begingroup\$

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.

\$\endgroup\$

migrated from stackoverflow.com Oct 6 '14 at 1:38

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ while(!inFile.eof()) is bad, see stackoverflow.com/questions/5605125/… instead use while (inFile >> words). Pass string to printWords as const reference. \$\endgroup\$ – Neil Kirk Oct 5 '14 at 22:41
  • \$\begingroup\$ 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': ... \$\endgroup\$ – Keugyeol Oct 5 '14 at 22:42
  • \$\begingroup\$ 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. \$\endgroup\$ – Paweł Stawarz Oct 5 '14 at 22:42
  • 2
    \$\begingroup\$ z = 'i' That's not confusing at all. :) \$\endgroup\$ – Greg Hewgill Oct 5 '14 at 22:43
  • 2
    \$\begingroup\$ Don't use plural forms ("words") for singular entities and don't name a function "printWords" if it doesn't print words. \$\endgroup\$ – molbdnilo Oct 5 '14 at 22:56
5
\$\begingroup\$

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";
    }
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ This answer needs its own review. \$\endgroup\$ – Martin York Oct 6 '14 at 5:45

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.