Removing duplication in horizontal and vertical checks

Question

// Check horizontals
for (int y = 0; y < sGridHeight; y++)
{
    Shape* groupShape = nullptr;
    int groupSize = 0;
    for (int x = 0; x < sGridWidth; x++)
    {
        GridItem* gridItem = GetItem(x, y);
        Shape* shape = gridItem->GetShape();
        if (shape == groupShape)
        {
            groupSize++;
            if (groupSize == m_MatchSize)
            {
                g_Audio->PlaySound(m_GemDestroyedSound.c_str());
                m_Score += shape->GetScore();
                for (int tx = x - m_MatchSize + 1; tx <= x; tx++)
                {
                    RANDOMIZE(GetItem(tx, y))
                }
                return;
            }
        }
        else
        {
            groupSize = 1;
            groupShape = shape;
        }
    }
}

// Check verticals
for (int x = 0; x < sGridWidth; x++)
{
    Shape* groupShape = nullptr;
    int groupSize = 0;
    for (int y = 0; y < sGridHeight; y++)
    {
        GridItem* gridItem = GetItem(x, y);
        Shape* shape = gridItem->GetShape();
        if (shape == groupShape)
        {
            groupSize++;
            if (groupSize == m_MatchSize)
            {
                g_Audio->PlaySound(m_GemDestroyedSound.c_str());
                m_Score += shape->GetScore();
                for (int ty = y - m_MatchSize + 1; ty <= y; ty++)
                {
                    RANDOMIZE(GetItem(x, ty))
                }
                return;
            }
        }
        else
        {
            groupSize = 1;
            groupShape = shape;
        }
    }
}

Is there an easy way to clean this up? It seems like a lot of code duplication.

I have no qualms about using #defines, and I cannot use C++11.

Answer 1

The most straightforward way to do this is to introduce a new boolean variable horz that is true in the case that you wish to check horizontals. With that simple addition, both routines now can be expressed as one:

// if (horz) is true, check horizontals, else check verticals
aLimit = horz ? sGridHeight : sGridWidth;
bLimit = horz ? sGridWidth : sGridHeight;
for (int a = 0; a < aLimit; a++)    
{
    Shape* groupShape = nullptr;
    int groupSize = 0;
    for (int b = 0; b < bLimit; b++)   
    {
        GridItem* gridItem = (horz ? GetItem(b, a) : GetItem(a, b));
        Shape* shape = gridItem->GetShape();  
        if (shape == groupShape)
        {
            groupSize++;
            if (groupSize == m_MatchSize)
            {
                g_Audio->PlaySound(m_GemDestroyedSound.c_str());
                m_Score += shape->GetScore();
                for (int tb = b - m_MatchSize + 1; tb <= b; tb++)  
                {
                    RANDOMIZE(horz ? GetItem(tb, a) : GetItem(a, tb));
                }
                return;
            }
        }
        else
        {
            groupSize = 1;
            groupShape = shape;
        }
    }
}

However, there are a number of things that suggest opportunities for further improvement. In particular, the use of pointers is probably not a good idea. For example, these two lines:

    GridItem* gridItem = (horz ? GetItem(b, a) : GetItem(a, b));
    Shape* shape = gridItem->GetShape();  

Are the only place that the gridItem pointer is used, and it's not checked for nullptr before it's dereferenced. So either that's a bug if GetItem() could possibly return a nullptr or it should instead be written as a reference. Even better, it could be combined into a single line and the variable eliminated completely.

Using this modified code is OK, but it's still a little unwieldy, so I would suggest that the GetItem() function should take a third argument which is the bool horz. That would make those same two lines of code look like this:

    Shape* shape = GetItem(a, b, horz).GetShape();  

Much nicer, but again, I'd turn this pointer into a reference (or better yet, a const reference if possible).

Answer 2

Consider replacing:

        groupSize++;
        if (groupSize == m_MatchSize)
        {
            g_Audio->PlaySound(m_GemDestroyedSound.c_str());
            m_Score += shape->GetScore();
            for (int tx = x - m_MatchSize + 1; tx <= x; tx++)
            {
                RANDOMIZE(GetItem(tx, y))
            }
            return;
        }

With something more like:

groupVector.push_back(gridItem);
if(groupVector.size() == m_MatchSize)
{
    DestroyGroup(groupVector);
    return;
}

// where DestroyGroup looks like:
void MyClass::DestroyGroup(const std::vector<GridItem*> & groupVector)
{
    g_Audio->PlaySound(m_GemDestroyedSound.c_str());
    m_Score += shape->GetScore();
    for(std::vector<GridItem*>::iterator itemIter = groupVector.begin();
        itemIter != groupVector.end();
        ++itemIter)
    {
        RANDOMIZE(*itemIter)
    }
    return;
}

I leave optimizing the code above and finding more appropriate function and variable names as an exercise for the original poster. But basically, if you maintain some kind of list of items that you've already traversed you should be able to more easily remove the code duplication that exists between the horizontal and vertical versions of your grid traversal.