While loop in C++ (using break)

Question

I'm currently working through the book C++ Primer (recommended on SO book list). An exercise was given that was essentially read through some strings, check if any strings were repeated twice in succession, if a string was repeated print which word and break out of the loop. If no word was repeated, print that. Here is my solution, I'm wondering a) if it's not a good solution and b) is my test condition for no repeated words ok? Because I had to add 1 to the variable to get it to work as expected. Here is my code:

#include <iostream>
#include <vector>
#include <string>

using namespace std;

int main() {


vector<string> words = {"Cow", "Cat", "Dog", "Dog", "Bird"};

string tempWord;
unsigned int i = 0;

while (i != words.size())
{
  if (words[i] == tempWord)
  {
    cout << "Loop exited as the word " << tempWord << " was repeated.";
    break;
  } 
  else 
  {
    tempWord = words[i];
  }
    // add 1 to i to test equality as i starts at 0
    if (i + 1 == words.size())
        cout << "No word was repeated.";

    ++i;

}

return 0;
}

The first time the loop runs it will test agains an empty string in tempWord. Then, it will only find the duplicates if they follow each other, so if your input array is e.g. {"Cow", "Dog", "Cat", "Dog", "Bird"} it won't find the duplicates. — Steeve
– Steeve, Commented Oct 6, 2016 at 9:27
Hmmmm, at a glance, it looks like you're only checking for contiguous repeats, is that what you wanted to achieve? — George
– George, Commented Oct 6, 2016 at 9:28
You code will not detect repeating word if they are separated. For example, "Cat", "Dog", "Cow", "Cat" — GMichael
– GMichael, Commented Oct 6, 2016 at 9:28
@George yeah the book said to just check for words twice in succession — MKSnazzy
– MKSnazzy, Commented Oct 6, 2016 at 9:31
Cool, the only things I would suggest would be initializing tempworld, breaking at ` if (i + 1 == words.size())` and maybe caching words.size() to guarantee that it's only called once, anything else is just getting even more nitpicky, it's a good attempt :) — George
– George, Commented Oct 6, 2016 at 9:35

UKMonkey · Accepted Answer · 2016-10-06 09:49:23Z

1

The definition of "good solution" will somewhat depend on the requirements - the most important will always be "does it work" - but then there may be speed and memory requirements on top.

Yours seems to work (unless you have the first string being blank, in which case it'll break); so it's certainly not that bad.

The only suggestion I could make is that you could have a go at writing a version that doesn't keep a copy of one of the strings, because what if they're really really big / lots of them and copying them will be an expensive process?

answered Oct 6, 2016 at 9:49

UKMonkey

7,1653 gold badges24 silver badges30 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

pingul · Accepted Answer · 2016-10-06 09:43:34Z

0

I would move the test condition outside of the loop, as it seems unnecessary to perform it at every step. For readability I would add a bool:

string tempWord;
unsigned int i = 0;
bool exited = false;

while (i != words.size())
{
  if (words[i] == tempWord)
  {
    cout << "Loop exited as the word " << tempWord << " was repeated.";
    exited = true;
    break;
  } 
  else 
  {
    tempWord = words[i];
  }
    ++i;
}

// Doing the check afterwards instead
if (!exited) 
{
    cout << "No word was repeated.";
}

answered Oct 6, 2016 at 9:43

pingul

3,4813 gold badges30 silver badges47 bronze badges

Comments

Steeve · Accepted Answer · 2016-10-06 09:38:37Z

a) if it's not a good solution

For the input specified it is a good solution (it works). However, tempWord is not initialized, so the first time the loop runs it will test against an empty string. Because the input does not contain an empty string, it works. But if your input started with an empty string it would falsely find as repeating.

b) is my test condition for no repeated words ok? Because I had to add 1 to the variable to get it to work as expected.

Yes, and it is simply because the indexing of the array starts from zero, and you are testing it against the count of items in the array. So for example an array with count of 1 will have only one element which will be indexed as zero. So you were right to add 1 to i.

alexeykuzmin0 · Accepted Answer · 2016-10-06 11:43:00Z

0

As an answer for the training task your code (after some fixes suggested in other answers) look good. However, if this was a real world problem (and therefore it didn't contain strange restrictions like "use a for loop and break"), then its writer should also consider ways of improving readability.

Usage of default STL algorithm is almost always better than reinventing the wheel, so I would write this code as follows:

auto equal = std::find_adjacent(words.begin(), words.end());
if (equal == words.end())
{
    cout << "No word was repeated" << endl;
}
else
{
    cout << "Word " << *equal << " was repeated" << endl;
}

answered Oct 6, 2016 at 11:43

alexeykuzmin0

6,4702 gold badges30 silver badges51 bronze badges

Collectives™ on Stack Overflow

While loop in C++ (using break)

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related