using ranges to filter side by side vectors while remaining readable

Question

So imagine i have 2 vectors (for reasons to do with the framework i am working in):

std::vector<bool> valid = {false, true, true, false};
std::vector<int> percentages = {2, 50, 3, 100};

These vectors represent a batteries validity and the percentage charge. I want to find the minimum charge of the valid batteries. To do this with ranges, the best i can come up with is:

auto min = std::get<1>(
    std::ranges::min(
        std::views::zip(valid, percentages) | 
        std::views::filter([](const auto& vals) {return std::get<0>(vals);}), 
    [](const auto& vals0, const auto& vals1) {
    return std::get<1>(vals0) < std::get<1>(vals1);
}));

Now this works but it is nearly impossible to read. So the for loop variety would be something like:

int min_percentage = 100;
for (const auto& [is_valid, percentage] : std::views::zip(valid, percentages)) {
    if (is_valid) {
        min_percentage = std::min(min_percentage, percentage);
    }
}

Which works just fine as well.

The second one is objectively better. So the question is, is there a way to write this with ranges whilst keeping the code readable?

Answer 1

In general, the reason that this is poorly readable:

auto min = std::get<1>(
    std::ranges::min(
        std::views::zip(valid, percentages) | 
        std::views::filter([](const auto& vals) {return std::get<0>(vals);}), 
    [](const auto& vals0, const auto& vals1) {
    return std::get<1>(vals0) < std::get<1>(vals1);
}));

is that it's the wrong algorithm - you want to find the minimum percentage but instead the algorithm you're doing is finding the minimum pair of (valid, percentage) by percentage.

Simply restructuring to the correct algorithm (the minimum percentage) is a big improvement:

auto min = 
    std::ranges::min(
        std::views::zip(valid, percentages)
        | std::views::filter([](const auto& vals) {return std::get<0>(vals);})
        | std::views::values);

And there's a proposal in flight (P2769) to let you write that lambda as std::ranges::get_key (or std::ranges::get_element<0>).

Note also that the predicate you passed into min wasn't strictly necessary - the default < would already do the right thing, since all of the tuples in that range are now (true, x), so the minimum value there is, by definition, the one with the minimum x.

---

This is one of those cases where it's like we have a missing algorithm:

ranges::min(
    views::zip(valid, percentages)
    | views::filter_map([](auto&& e) -> optional<int> {
        auto& [valid, perc] = e;
        if (valid) {
            return perc;
        } else {
            return nullopt;
        }
    })
);

Where filter_map takes a T -> optional<U> and yields a range of U.

In this case though, you can kind of cheat - the exact same algorithm you wrote with your for loop you can accomplish with a transform: you're mapping invalid batteries to 100:

ranges::min(
    views::zip(valid, percentages)
    | views::transform([](auto&& e) {
        auto& [valid, perc] = e;
        return valid ? perc : 100;
    })
);

or, more directly:

ranges::min(
    views::zip_transform([](bool valid, int perc){
        return valid ? perc : 100;
    }, valid, percentages)
);

This has a different semantic in the case where you have no valid batteries (you get a well-defined 100 instead of UB). Although your loop is still well-defined even in the case where there are no batteries (you get 100) whereas here the empty case is still UB.