patterncppMinor
Sequentially remove members "repelled" by previous members
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.
Possible implementation:
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;
}
#endifPossible 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
Right now, the lambda in your predicate has values
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:
Consider encapsulating the
It's a bit odd to have to use
Check for empty container in
I tried to exercise the
I was rewarded with a core dump. It doesn't seem to like an empty destination container.
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
Use
const references for the predicateRight 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
eraseIt'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_repelledI 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.