4
\$\begingroup\$

I am new to C++, learning from the book primer C++

The exercise:

Write a program to read strings from standard input looking for duplicated words. The program should find places in the input where one word is followed immediately by itself. Keep track of the largest number of times a single repetition occurs and which word is repeated. Print the maximum number of duplicates, or else print a message saying that no word was repeated.

vector<string> words;
string w, highest_occ_word;
int occ = 1, highest_occ = 0;

while (cin >> w)
    words.push_back(w);

for (auto i = words.begin(); i != words.end(); ++i)
{
    if ((i + 1) != words.end() && *i == *(i + 1))
    {
        ++occ;
        if (highest_occ < occ)
        {
            highest_occ = occ;
            highest_occ_word = *i;
        }
    }
    else
        occ = 1;            
}

cout << highest_occ_word << " occured for " << highest_occ << " times \n";

Is this good code? If not, what can I do to improve it?

\$\endgroup\$
3
\$\begingroup\$

You loop from the first element to the last, and in each iteration you check if a next element exists or not. This check will only be false on the last element, it will be true for all elements before that. In other words, most of the time the check is unnecessary.

To get rid of this redundancy, you could start the iteration from the second element, and make comparisons with the previous one, which always exists. No more redundant checks.

More importantly, keep in mind that you don't need to read the entire input into memory to find the most frequent repetition. You don't need a vector. You could just parse the input word by word, always keeping only the last word in memory for comparisons. That way you will drastically reduce the memory footprint of the program.

\$\endgroup\$
1
\$\begingroup\$

Iterate over the stream:

There is an iterator that acts on a stream (treating the stream like a container). So you don't need to read the data into a container and then parse the container.

Replace:

while (cin >> w)
    words.push_back(w);

for (auto i = words.begin(); i != words.end(); ++i)
{
    // STUFF
}

With:

using WordIter = std::istream_iterator<std::string>;
for(auto i = WordIter(std::cin); i != WordIter(); ++i)
{
    // STUFF
}

One Variable Per Line:

Declare only one variable per line:

string w, highest_occ_word;
int occ = 1, highest_occ = 0;

Also declare variables as close to the point of first use as you can there is no point in declaring them all at the top. This has a couple of advantages. 1) You can see the type. 2) There constructors are not called if you never get to the point of declaration. 3) It makes the code more logical and less cluttered (PO).

Prefer look back

Not sure I like your look forward approach.

for (auto i = words.begin(); i != words.end(); ++i)
{
    if ((i + 1) != words.end() && *i == *(i + 1))

I would use the current iterator with state I had saved from the previous iteration.

std::string     wordOfLongestSeq;
std::size_t     countOfLongestSeq = 1; // have to beat 1.

std::string     currentWord;
std::size_t     courrentLength;

using WordIter = std::istream_iterator<std::string>;
for(auto loop = WordIter(std::cin); loop != WordIter(); ++loop)
{
    std::string const& word = *loop;
    // If the new word does not match the current word.
    // This works for the first word (as current word is "")
    if (word != currentWord) {

        // We are switching words.
        // But before we do check to see if it broke
        // the previous record.
        if (courrentLength > countOfLongestSeq) {
            countOfLongestSeq = courrentLength;
            wordOfLongestSeq  = currentWord;
        }

        // Reset the current word.
        currentWord    = word;
        courrentLength = 0;
    }
    // Increment the count of words.
    ++courrentLength;
}
// Check to see if the last word
// broke the record.
if (courrentLength > countOfLongestSeq) {
    countOfLongestSeq = courrentLength;
    wordOfLongestSeq  = currentWord;
}

Check your requirements:

Print the maximum number of duplicates, or else print a message saying that no word was repeated

Looks like you don't need fulfill the else portion from the requirements.

cout << highest_occ_word << " occured for " << highest_occ << " times \n";
\$\endgroup\$

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.