-1
\$\begingroup\$

So I'm in the process of making a simple bejeweled clone and for some reason my "find matches" function is having problems. I've broken it up so it check horizontally, then vertically and right now I'm just working with the first. At the end it should print out the gems and their coordinates (for now) but it only prints out the final gem in the matches, for every gem in that line of matches. Here's the function code. If more is needed let me know.

EDIT: Here's updated code. It should run faster due to less looping but I still get the same problem. I've also tried to add the gameobject's directly from the array but the row and col values (that otherwise are initialized) have 0 for all the values. Thanks to anyone who has an idea. Its so frustrating I'm stuck on something so simple.

    void RowCheck()
{
    for (int row = 0; row < gridSize; row++)
    {
        for(int col = 0; col < gridSize - 2; col++)
        {
            if(col + 2 < gridSize)
            {
                if (GemGrid[row, col] == GemGrid[row, col + 1] && GemGrid[row, col] == GemGrid[row, col + 2])
                {
                    for (int x = col + 2; col <= x; col++)
                    {
                        GameObject tempGem = new GameObject();
                        tempGem = GemGrid[row,col];
                        tempGem.GetComponent<ForGems>().row = row;
                        tempGem.GetComponent<ForGems>().col = col;
                        MatchedGems.Add(tempGem);
                    }
                    if (col + 1 <= gridSize)
                    {
                        if (GemGrid[row, col] == GemGrid[row, col + 1])
                        {
                            col++;
                            GameObject fourthGem = GemGrid[row,col];
                            fourthGem.GetComponent<ForGems>().row = row;
                            fourthGem.GetComponent<ForGems>().col = col;
                            MatchedGems.Add(fourthGem);

                            if (col + 1 <= gridSize)
                            {
                                if (GemGrid[row, col] == GemGrid[row, col + 1])
                                {
                                    col++;
                                    GameObject fifthGem = GemGrid[row,col];
                                    fifthGem.GetComponent<ForGems>().row = row;
                                    fifthGem.GetComponent<ForGems>().col = col;
                                    MatchedGems.Add(fifthGem);
                                }
                            }
                        }
                    }
                }
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ One improvement: Don't use i and j for variable names if you are iterating over a grid. Instead use more descriptive names. Iterate with variables named x and y instead. \$\endgroup\$ – Lysol Feb 7 '14 at 17:53
  • \$\begingroup\$ All the other ones I have use row and col. Looking back I don't know why this one I used i and j but I'll be fixing that this weekend probably \$\endgroup\$ – Matt Griffin Feb 7 '14 at 17:56
  • \$\begingroup\$ Incrementing i and j within your loop, and mixing the use i/j and x/y is a sure way to irritate your readers (including yourself in a couple of months). \$\endgroup\$ – Vaillancourt Feb 7 '14 at 18:13
  • 1
    \$\begingroup\$ You should post this code-review after you have finished debugging, because - I'm going to be blunt - the code is awful. They will recommend that you extract portions of this into smaller methods. \$\endgroup\$ – BlueRaja - Danny Pflughoeft Feb 7 '14 at 19:37
  • \$\begingroup\$ Not to mention, in Unity, you really don't want to be doing GetComponent<> in a loop... or even in the game loop. \$\endgroup\$ – Runonthespot Feb 11 '14 at 23:34
1
\$\begingroup\$

Edit

As posted in your OP's comments, your code could be better:

  1. There is a useless if.
  2. There is copy-pasted code which should be extracted.
  3. The for you use to add your 3 first gems to the list is really hard to follow/read.
  4. In GameObject tempGem = new GameObject(); the new GameObject() is useless; it will eat up process and garbage collector.
  5. You change the value of the iterator (col) within the for which is quite generally unexpected and makes the code harder to follow.

Here is a cleaned up version:

void RowCheck()
{
    for (int row = 0; row < gridSize; row++)
    {
        int col = 0;
        while(col < gridSize - 2)
        {
            if (GemGrid[row, col] == GemGrid[row, col + 1] && GemGrid[row, col] == GemGrid[row, col + 2])
            {
                addMatch( GemGrid[row, col] );
                col++; // Index + 1
                addMatch( GemGrid[row, col] );
                col++; // Index + 2
                addMatch( GemGrid[row, col] );
                col++; // Index + 3
                if (col < gridSize && GemGrid[row, col - 1] == GemGrid[row, col])
                {
                    addMatch( GemGrid[row, col] );
                    col++; // Index + 4
                    if (col < gridSize && GemGrid[row, col - 1] == GemGrid[row, col])
                    {
                        addMatch( GemGrid[row, col] );
                        col++; // Index + 5
                    }
                }
            }
        }
    }
}

Notice that I have changed the inner for for a while which implies that the iterator variable is managed within it, not by the for construct. The way it is now, it prepares for the next loop iteration.

If you still have your issue, I encourage you to use a debugger or prints to console to see if your data is properly added to your MatchedGems, and, if they are, check if your problem comes from your container.

\$\endgroup\$
  • \$\begingroup\$ Sorry, still the same, that code is far improved in terms of efficiency though so thanks for that. \$\endgroup\$ – Matt Griffin Feb 9 '14 at 19:05
  • \$\begingroup\$ If you have new improved code, you should edit your post and update your code there! \$\endgroup\$ – Vaillancourt Feb 9 '14 at 19:31
  • \$\begingroup\$ edited my original post \$\endgroup\$ – Matt Griffin Feb 10 '14 at 0:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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