HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Slow foreach loop to check for intersect

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
loopintersectslowforforeachcheck

Problem

I'm trying to make a loop where it returns whether or not a rectangle collides and then to which coords the rectangle should move to.

I'm calling my function like this:

rectangles2[i].rect = rectangleupdated(i, rectangles2);


And this is inside my function:

private Rectangle rectangleupdated(int i, List rectangles2)
{
    //fills rectangles with rectangles to check intersect with
    BOTTOM = rectangles2[i].rect;
    BOTTOM_LEFT = rectangles2[i].rect;
    BOTTOM_RIGHT = rectangles2[i].rect;
    //bool to be set to false if intersects returns true
    bBOTTOM = true;
    bBOTTOM_LEFT = true;
    bBOTTOM_RIGHT = true;
    //move the intersect rectangles
    BOTTOM.Y++;
    BOTTOM_LEFT.Y++;
    BOTTOM_LEFT.X--;
    BOTTOM_RIGHT.Y++;
    BOTTOM_RIGHT.X++;

    foreach (rectList rects in rectangles2)
    {
        if (!rects.type.Contains("bg"))
        {
            if (rectangles2[i].rect.Y < 500)
            {
                if (BOTTOM.IntersectsWith(rects.rect))
                {
                    bBOTTOM = false;
                }
                else if (BOTTOM_LEFT.IntersectsWith(rects.rect))
                {
                    bBOTTOM_LEFT = false;
                }
                else if (BOTTOM_RIGHT.IntersectsWith(rects.rect))
                {
                    bBOTTOM_RIGHT = false;
                }
            }
            else
            {
                return rectangles2[i].rect;
            }
        }
    }
    if (bBOTTOM == true) return BOTTOM;
    else if (bBOTTOM_RIGHT == true) return BOTTOM_RIGHT;
    else if (bBOTTOM_LEFT == true) return BOTTOM_LEFT;
    return rectangles2[i].rect; 
}


I think I know a little bit why it's lagging so hard but I don't know how to fix it / make this code more usable.

Solution

You're not using the C# naming convention:

class ClassNamesShouldBePascalCase
{
    const CONTANTS_ARE_IN_ALL_CAPS;
    private _membersShouldStartWithAUnderScoreAndBeCamelCase; //_camelCase
    public PropertiesShouldBePascalCase {get; set;} //PascalCase

    public void PublicMethodsShouldBePascalCase
    {
        object methodLevelVariablesShouldBeCamelCase; //camelCase
    }
}
//The underscore for private variables is optional, but good practice.


Is there a reason you are using a custom class called rectList instead of using a List, besides the type ? If there are more reasons, I recommend calling the class Rectangles, otherwise, just manage the Type separately.

Don't call your parameter rectangles2 ... rectangles, or values is a good name.

if (rectangles2[i].rect.Y < 500)


500 is a magic number, and should be declared somewhere else as a constant with a useful name, so that those reading it know what it is for.

Give your booleans more descriptive names like:

bool bottomHasCollision


bool hasCollisionBottom


Use the ternary operator to clean up your final return statement:

return bBOTTOM       ? BOTTOM :
       bBOTTOM_RIGHT ? BOTTOM_RIGHT :
       bBOTTOM_LEFT  ? BOTTOM_LEFT :
       rectangles2[i].rect;


If you could fix these things, and upload another question I'd be more inclided to review the performance of the program, however I will need to know more about the program and see some of the other classes. You should try saving the DateTime.Now at the beginning and end of the method, and various other places inside to track the performance to pinpoint what is taking so long. There is also software out there that can help you do that.

Edit:

I've taken the liberty to implement the code changes I was talking about above. Plus some others to make the code more concise.

private Rectangle Rectangleupdated(List rectangles, int i)
{
    Rectangle rect = rectangles[i].Rectangle;

    //fills rectangles with rectangles to check intersect with
    bottom = bottomLeft = bottomRight = rect;

    //bool to be set to false if intersects returns true
    bottomFlagged = bottomLeftFlagged = bottomRightFlagged = true;

    //move the intersect rectangles
    bottom.Y++;

    bottomLeft.Y++;
    bottomLeft.X--;

    bottomRight.Y++;
    bottomRight.X++;

    foreach (Rectangles rects in rectangles)
    {
        if (rects.Type.Contains("bg") || rect.Y >= 500) continue;

        bottomFlagged     = bottomFlagged && !bottom.IntersectsWith(rects.Rectangle);
        bottomLeftFlagged = bottomLeft    && !bottomLeft.IntersectsWith(rects.Rectangle);
        bottomRight       = bottomRight   && !bottomRight.IntersectsWith(rects.Rectangle);
    }
    return bottomFlagged      ? bottom :
           bottomRightFlagged ? bottomRight :
           bottomLeftFlagged  ? bottomLeft :
           rect;
}


I would however like to see your Rectangle and rectList classes, to understand where the pain points might be.

Code Snippets

class ClassNamesShouldBePascalCase
{
    const CONTANTS_ARE_IN_ALL_CAPS;
    private _membersShouldStartWithAUnderScoreAndBeCamelCase; //_camelCase
    public PropertiesShouldBePascalCase {get; set;} //PascalCase

    public void PublicMethodsShouldBePascalCase
    {
        object methodLevelVariablesShouldBeCamelCase; //camelCase
    }
}
//The underscore for private variables is optional, but good practice.
if (rectangles2[i].rect.Y < 500)
bool bottomHasCollision
bool hasCollisionBottom
return bBOTTOM       ? BOTTOM :
       bBOTTOM_RIGHT ? BOTTOM_RIGHT :
       bBOTTOM_LEFT  ? BOTTOM_LEFT :
       rectangles2[i].rect;

Context

StackExchange Code Review Q#49563, answer score: 7

Revisions (0)

No revisions yet.