patterncsharpMinor
Slow foreach loop to check for intersect
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:
And this is inside my function:
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.
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:
Is there a reason you are using a custom class called
Don't call your parameter
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:
Use the ternary operator to clean up your final
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.
I would however like to see your
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 bottomHasCollisionbool hasCollisionBottomUse 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 bottomHasCollisionbool hasCollisionBottomreturn 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.