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

Remove code duplication inside of a loop preserving performance

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

Problem

I'm coding a 2D collision engine, and I need to merge adjacent axis-aligned bounding boxes depending on a direction (left, right, top, bottom).

The four cases are very similar, except for the if condition, and the push_back argument.
Is there any way I can refactor this code without compromising performance?

```
vector getMergedAABBSLeft(vector mSource)
{
vector result;

while(!mSource.empty())
{
bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

for(auto& b : mSource)
if(a.getRight() == b.getRight())
{
result.push_back(getMergedAABBVertically(a, b));
eraseRemove(mSource, b); merged = true; break;
}

if(!merged) result.push_back(a);
}

return result;
}

vector getMergedAABBSRight(vector mSource)
{
vector result;

while(!mSource.empty())
{
bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

for(auto& b : mSource)
if(a.getLeft() == b.getLeft())
{
result.push_back(getMergedAABBVertically(a, b));
eraseRemove(mSource, b); merged = true; break;
}

if(!merged) result.push_back(a);
}

return result;
}

vector getMergedAABBSTop(vector mSource)
{
vector result;

while(!mSource.empty())
{
bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

for(auto& b : mSource)
if(a.getBottom() == b.getBottom())
{
result.push_back(getMergedAABBHorizontally(a, b));
eraseRemove(mSource, b); merged = true; break;
}

if(!merged) result.push_back(a);
}

return result;
}

vector getMergedAABBSBottom(vector mSource)
{
vector result;

while(!mSource.empty())
{
bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

for(auto& b : mSource)
if(a.getTop() == b.getTop())
{

Solution

I find this line hard to read:

bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();


Please split variables up 1 per line.

Yes the new syntax allows {} for list initialization. But these are not lists so it seems confusing to me (this one is more personal bias so feel free to ignore).

bool   merged(false);
AABB   a(mSource.back());
mSource.pop_back();


Pass large parameters by const reference if you can.

It saves a copy and you don't seem to need the copy (since you are not modifying the value (Note the eraseRemove() call has no effect externally and does not effect the rest of the code)).

vector getMergedAABBSLeft(vector const&   mSource)
                                     //     ^^^^^^^^


Since your four functions are identical apart from one method call you could write a generic version and pass that method as a parameter:

vector getMergedAABBSLeft(vector mSource)
{    return getMergedAABBSGeneric(mSource, &AABB::getRight);
}

Code Snippets

bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();
bool   merged(false);
AABB   a(mSource.back());
mSource.pop_back();
vector<AABB> getMergedAABBSLeft(vector<AABB> const&   mSource)
                                     //     ^^^^^^^^
vector<AABB> getMergedAABBSLeft(vector<AABB> mSource)
{    return getMergedAABBSGeneric(mSource, &AABB::getRight);
}

Context

StackExchange Code Review Q#22817, answer score: 3

Revisions (0)

No revisions yet.