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

Sequentially remove members "repelled" by previous members

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

Problem

I was working on a project that need to remove extra data such that the remains are not "too close" to each other, and the algorithm should be as generalize as possible. To have more general usage, I try to write in std format.

I'm not sure if the algorithm is the best way to accomplish the goal, or if the style of coding is optimal.

#ifndef _REPEL
#define _REPEL

#include 
#if __cplusplus >= 201103L
#include 
#endif

#if __cplusplus >= 201103L
template
bool check_repelled(ForwardIt first, ForwardIt last, InputIt target, BinaryPredicate p){
    return last != std::find_if(first, last, [p, target](decltype(*first) i)->bool{ return p(i, *target); });
}
#else
template
bool check_repelled(ForwardIt first, ForwardIt last, InputIt target, BinaryPredicate p){
    for(ForwardIt i = first; i != last; ++i)
        if(p(*i, *target))
            return true;
    return false;
}
#endif

template
ForwardIt remove_copy_repelled(InputIt first, InputIt last, ForwardIt d_first, BinaryPredicate p){
    ForwardIt d_end = d_first;
    for(; first != last; ++first){
        if(!check_repelled(d_first, d_end, first, p))
            *d_end++ = *first;
    }
    return d_end;
}

template
ForwardIt remove_repelled(ForwardIt first, ForwardIt last, BinaryPredicate p){
    ForwardIt i, j;
    for(i = first; i != last; ++i)
        if(check_repelled(first, i, i, p))
            break;
    if(i == last)
        return last;
    for(j = i; ++j != last;)
        if(!check_repelled(first, i, j, p))
#if __cplusplus >= 201103L
            *i++ = std::move(*j);
#else
            std::iter_swap(i++, j);
#endif
    return i;
}
#endif


Possible implementation:

std::vectorx = {1,3,2,8,5,6,7};
x.erase(remove_repelled(x.begin(), x.end(), [](int a,int b)->bool{std::abs(a-b) < 2;}),x.end());
//x == {1,3,8,5}

Solution

In all, there's not much to complain about, but I did find a few things that might be improved. All of my comments and testing are about C++11 mode, since that's what I have conveniently available.

Use const references for the predicate

Right now, the lambda in your predicate has values a and b passed by value, which is fine for primitive types such as int but could be relatively costly for classes such as std::complex due to all of the overhead of construction and destruction of temporary objects. With that said, it's only a minor change since std::abs creates a temporary itself, and this is a quibble with your example use of the the template rather than the template itself. This is what I used for testing:

typedef std::complex mytype;
auto pred = [](const mytype &a,const mytype &b)->bool{
    return abs(a-b) < 2;
};


Fix the C++11 version of the code

As has been mentioned in comments, the syntax isn't quite right for the C++ version of the lambda in the template. It should be:

[p, target](decltype(*target) i)->bool{return p(i, *target); }


Consider encapsulating the erase

It's a bit odd to have to use x.erase() outside of the templated function. Putting it inside would make usage a little simpler.

Check for empty container in remove_copy_repelled

I tried to exercise the remove_copy_repelled code. The first thing I tried was this:

std::vectorx = {1,3,2,8,5,6,7};
std::vectorx2;
x2.erase(remove_copy_repelled(
    x.cbegin(), x.cend(), x2.begin(), pred), x2.end());


I was rewarded with a core dump. It doesn't seem to like an empty destination container.

std::vectorx2;
x2.reserve(x.size());


This worked, but isn't particularly general, so you could do a template specialization, but there may be a better way. In particular, you might consider constructing the destination container, perhaps using a passed parameter that is a placeholder for push_back or whatever the caller wishes to use to add another object to the container.

Code Snippets

typedef std::complex<double> mytype;
auto pred = [](const mytype &a,const mytype &b)->bool{
    return abs(a-b) < 2;
};
[p, target](decltype(*target) i)->bool{return p(i, *target); }
std::vector<mytype>x = {1,3,2,8,5,6,7};
std::vector<mytype>x2;
x2.erase(remove_copy_repelled(
    x.cbegin(), x.cend(), x2.begin(), pred), x2.end());
std::vector<mytype>x2;
x2.reserve(x.size());

Context

StackExchange Code Review Q#53953, answer score: 3

Revisions (0)

No revisions yet.