Should I avoid using goto here? If so, how?

Question

I am coding for a function that takes a hand and checks for pairs:

int containsPairs(vector<Card> hand)
{
    int pairs{ 0 };

    loopstart:
    for (int i = 0; i < hand.size(); i++)
    {
        Card c1 = hand[i];
        for (int j = i + 1; j < hand.size(); j++)
        {
            Card c2 = hand[j];
            if (c1.getFace() == c2.getFace())
            {
                pairs++;
                hand.erase(hand.begin() + i);
                hand.erase(hand.begin() + (j - 1));
                goto loopstart;
            }
        }
    }
    return pairs;
}

When it finds pair on line 10, I want to delete the cards in the hand it found the pair with and then restart the whole loop with the deleted cards to find a second pair, if there is one. For me, goto was the most intuitive way to do this, but in this case, is that true?

Answer 1

Try this:

int containsPairs(vector<int> hand)
{
    int pairs{ 0 };

    for (int i = 0; i < hand.size(); i++)
    {
        int c1 = hand[i];
        for (int j = i + 1; j < hand.size(); j++)
        {
            int c2 = hand[j];
            if (c1 == c2)
            {
                pairs++;
                hand.erase(hand.begin() + i);
                hand.erase(hand.begin() + (j - 1));
                i--;
                break;
            }
        }
    }
    return pairs;
}

This is almost your version, the only difference is that instead of goto, there is i--; break;. This version is more efficient than yours, as it only does the double-loop once.

Is it more clear? Well, that's a personal preference. I'm not against goto at all, I think its current "never use it" status should be revised. There are occasions where goto is the best solution.

---

Here's another one, even simpler solution:

int containsPairs(vector<int> hand)
{
    int pairs{ 0 };

    for (int i = 0; i < hand.size(); i++)
    {
        int c1 = hand[i];
        for (int j = i + 1; j < hand.size(); j++)
        {
            int c2 = hand[j];
            if (c1 == c2)
            {
                pairs++;
                hand.erase(hand.begin() + j);
                break;
            }
        }
    }
    return pairs;
}

Basically, when it finds a pair, it removes only the farther card, and breaks the loop. So there is no need to be tricky with i.