Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
// 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.

share|improve this question
1  
I cannot use C++11 - nullptr is in C++11. –  Jamal May 8 at 19:25
    
Good point, I was unclear - I can use C++11, but not the C++11 standard library. –  Bertie Wheen May 8 at 20:57

2 Answers 2

up vote 3 down vote accepted

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).

share|improve this answer

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.

share|improve this answer
    
Since the DestroyGroup function doesn't care about the positions of any of the GridItems it can be used in future algorithms that locate GridItems in patterns that aren't vertical or horizontal, such as diagonal, or X-shaped patterns of items. –  YoungJohn May 8 at 20:24
1  
And if, in the future, the code starts to use C++11 and eliminate those pointers, the for loop gets even cleaner: for (auto &item : groupvector) RANDOMIZE(item); –  Edward May 8 at 21:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.