Skip to main content
Code Review

Return to Revisions

2 of 6
added 1071 characters in body
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

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:

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

 inFile.close();

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::string word;
 while(infile >> word)
 {
 std::cout << WordInfo(word) << "\n";
 }
 // Actually I would replace this loop with std::copy
 /*
 std::copy(std::istream_iterator<std::string>(infile),
 std::istream_iterator<std::string>(),
 std::ostream_iterator<WordInfo>(std::cout, "\n")
 );
 */
}
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
default

AltStyle によって変換されたページ (->オリジナル) /