c++ Question with displaying text between two array variables

Question

I always seem to have this problem when I want to display two variables of an array in one iteration of a loop, I'm wondering if there is a nicer way of displaying the array.

The code works if I change ++i to i+1 but I'm wondering why that happens, and I'm wondering if there's a smarter way to do what I want. The error says unsequenced modification and access to 'i' [-Wunsequenced]

#include <iostream>
#include <memory>
#include <string>
#include <algorithm>
using namespace std;

int main()
{
    int players = 0,rounds=0;
    bool winnerFound=false;
    cout << "Welcome to my tournament match maker\n";
    cout << "Enter the number of participants in the tournament: ";
    cin >> players;
    rounds=players/2;
    string *pNames = new string[players]; //dynamically allocate 
    for (int i = 0;i < players;i++)
    {
        cout << "Enter the name of player " << i + 1<<":";
        cin >> pNames[i];
    }
        random_shuffle(&pNames[0],&pNames[players]);
        for(int i=0;i<rounds;i++)
        {
           cout<<pNames[i]<<" vs "<<pNames[++i]; //
        }
        return 0;
}

I'm not sure if that's the best way to do it, any tips would be appreciated.

Possible duplicate of [Undefined behavior and sequence points](https://stackoverflow.com/questions/4176328/undefined-behavior-and-sequence-points) — taskinoor, May 27 '19 at 06:36
*"The code works if I change ++i to i+1 but I'm wondering why that happens"* You'r increasing `i` in `for(int i=0;i — Blaze, May 27 '19 at 06:36
It is intended, I want to display two different strings and not only have 'i' go up by one because itll show a name too many times if I don't. I don't see a difference from that code and this code that works for(int i=0;i — MikeCalicoder, May 27 '19 at 06:42
Please, note that there are two increments: pre-increment (`++i`) and post-increment (`i++`). The former increments value of `i` and returns the new value. The latter returns the value of `i` and increments afterwards (before next sequence point). — Scheff's Cat, May 27 '19 at 06:50
Yeah thats why i put ++i and not i++, and thats why im wondering why theres an issue. — MikeCalicoder, May 27 '19 at 06:52
@MikeCalicoder: for example, you will get 3 players, in the second loop you will get `i = 2`, and when you will `++i`, you're trying to access `pNames[3]`, but `pNames` have only 3 values (from 0 to 2). — Raffallo, May 27 '19 at 07:03
@Raffallo No, because the loop runs until `rounds`, and 3/2 == 1, so the loop will run only once, he will not try to access an extra element but rather miss the last one. — Fareanor, May 27 '19 at 07:09

Fareanor · Answer 1 · 2019-05-27T07:44:52.583

1

For the tips, I think it is better to use a STL container instead of a raw array. std::array or std::vector for example. You can change the declaration of pNames as follows:

std::vector <std::string> pNames(players);

Your problem is that you iterate only until rounds. You don't want that, you want to loop over all players, not only half of them.

To make your code work, you need to write:

for(size_t i = 0; !pNames.empty() && i < pNames.size()-1; ++i)
{
    std::cout << pNames[i] << " vs " << pNames[++i] << std::endl;
}

But I think it is a bad way to modify i from inside the loop, you better have to directly increment i by two in the loop statement. This will give:

for(size_t i = 0; !pNames.empty() && i < pNames.size()-1; i+=2)
{
    std::cout << pNames[i] << " vs " << pNames[i+1] << std::endl;
}

Of course, if you have only one player or any odd number of players, you will miss the last one (because you need two opponents to make a vs).

I hope it can help.

edited May 27 '19 at 07:44

answered May 27 '19 at 07:22

Fareanor

5,900
2
11
37

why check for `pNames.empty()` when the loop would abort with condition being `0 < 0-1` with i = 0 and size = 0? – FalcoGer May 27 '19 at 12:18
@FalcoGer Because with a `size_t` aka `unsigned long long`, 0-1 will underflow. And I didn't use an `int` in order to use the proper types the vector is expecting (and avoid either warnings for automatic conversion or `static_cast`s). – Fareanor May 27 '19 at 12:20
To be more clear, 0-1 equals -1 with `signed` types. But with `unsigned` types, it underflows and usually equals the max positive value the corresponding type can hold. So the condition will never be `false`. – Fareanor May 27 '19 at 12:25
I said `size_t` is a `unsigned long long` but it depends of the platform. But usually, `size_t` is defined as `unsigned` (`unsigned short`, `unsigned int`, ...) – Fareanor May 27 '19 at 12:45
Thank you the i+=2 is what is was looking for – MikeCalicoder May 28 '19 at 04:31

score 0 · Answer 2 · answered May 27 '19 at 10:06

#include <iostream>
#include <memory>
#include <string>
#include <algorithm>
#include <ctime>
using namespace std;
int main()
{
    srand(time(NULL));
    int players = 0,count=0;
    cout << "Welcome to my tournament match maker\n";
    cout << "Enter the number of participants in the tournament: ";
    cin >> players;
    string *pArray = new string[players]; //dynamically allocate
    cin.ignore();
    for (int i = 0;i < players;i++)
    {
        cout << "Enter the name of player " << i + 1<<":";
        getline(cin, pArray[i]);
    }
    random_shuffle(&pArray[0], &pArray[players]);
    for (int i = 0;i < players;i++)
    {
        cout << ++count << ": " << pArray[i] << " vs " << pArray[i + 1] << endl;
        i++;
    }

    return 0;
}

Any critiques?

You do a lot of strange things to be honest. Read my answer, it will provide you some tips **and** the solution to your problem. — Fareanor, May 27 '19 at 11:26
since you want to increment at the end of the loop anyway, you might as well increment by 2 in the loop header. generally you'd use a for loop for such scenarios where the entire control can happen in the for instruction — FalcoGer, May 27 '19 at 12:07

FalcoGer · Answer 3 · 2019-05-27T12:04:01.370

-1

Firstly i++ and ++i are different.
i++ increments after returning the value and
++i increments before returning the value of the result.

It would be more readable if you incremented your i in the for loop by two and then not have a side effect when asking for the array value. And of course you need to check if i is valid at this point

for(int i=0; i < players-1; i += 2)
{
    // check if valid.
    // with odd numbers of players, the last remaining player will be dropped.
    cout << pNames[i]<<" vs " << pNames[i+1]; // no side effect.
                                                  // In a for loop
                                                  // all increments should happen in
                                                  // the loop header.
}

edited May 27 '19 at 12:04

answered May 27 '19 at 07:44

FalcoGer

2,278
1
12
34

This method is wrong, let's consider 10 players. At the first iteration, you will display 0 and 1, but at the second iteration, you will display 4 and 5, you miss 2 and 3. – Fareanor May 27 '19 at 07:49
I see, but I'm not the one who downvoted your answer so I connot remove it. – Fareanor May 27 '19 at 11:42
But there is still an error, you should iterate while `i < players-1`. – Fareanor May 27 '19 at 11:49
@Fareanor how about you remove your downvote and i delete the post since you already answered? – FalcoGer May 27 '19 at 12:03
I have never downvoted you. This is not me, so I cannot remove the downvote. But yes, I already gave the solution and was the first to answer :) – Fareanor May 27 '19 at 12:11

c++ Question with displaying text between two array variables

3 Answers3