Code Review
Code is good, except the naming. There are some pieces which could be improved
Use auto
for iterators, as exact iterator type is not important, but it's properties are. Iterator properties are usually inherited from container it came from.
Use better constructor, there seems to be one which accepts pair of iterators, and in this case, pair of pointers denoting contiguous range.
Better (?) approach
The problem seems to be somewhat specific case of trimming in general. Note that trimming from the right and trimming from the left are symmetric, and std::reverse_iterator
"normalizes" trimming from right (great improvements thanks to @Toby).
template <typename BidirIterator,
typename Predicate>
auto trim(BidirIterator first, BidirIterator last, Predicate predicate) {
auto left_edge = std::find_if_not(first, last, predicate);
auto right_edge = std::find_if_not(std::reverse_iterator(last),
std::reverse_iterator(left_edge),
predicate).base();
return std::pair{left_edge, right_edge};
}
When reversing a range, first
becomes last
, and vice versa, that is why std::reverse_iterator
arguments were swapped. There is also C++17 feature in play (template class argument deduction).
Full code:
#include <utility>
#include <iterator>
#include <algorithm>
template <typename BidirIterator,
typename Predicate>
auto trim(BidirIterator first, BidirIterator last, Predicate predicate) {
auto left_edge = std::find_if_not(first, last, predicate);
auto right_edge = std::find_if_not(std::reverse_iterator(last),
std::reverse_iterator(left_edge),
predicate).base();
return std::pair{left_edge, right_edge};
}
#include <vector>
#include <stdexcept>
int main() {
std::vector<int> initial_values{2, 2, 3, 4, 5, 6};
auto predicate = [](int x) { return x % 2 == 0; };
auto [new_first, new_last] = trim(initial_values.begin(),
initial_values.end(),
predicate);
std::vector<int> resulting_values(new_first, new_last);
std::vector<int> correct_result{3, 4, 5};
if (resulting_values != correct_result) {
throw std::logic_error("incorrect trimming occured");
}
}
Demo on Wandbox.
To make trimming a little bit easier to use, one might write something like this:
template <typename Container, typename Predicate>
Container trim_copy(Container&& container, Predicate predicate) {
auto [new_first, new_last] = trim(container.begin(), container.end(),
predicate);
return Container(new_first, new_last);
}
and pass a lambda calling the NumericVector::is_na
:
auto predicate = [](auto x) { return NumericVector::is_na(x); };
or may be pointer to the function directly.