Given

• std::vector<int> vec1 of size s_vec and capacity c.
• std::vector<int> vec2.
• std::map<int, int> m of size s_m >= s_vec.
• std::unordered_set<int> flags.
• bool flag = False

I want to copy as many values of m (in order) into vec1 (overwriting previous values) without exceeding the capacity c. If any values remain I want to push those values to the end of vec2. For each of these, values I want to check if they are in flags. If they are, I'd like to set flag to true.

This is how I currently, achieve this:

  int i = 0;
  for (auto const& e : m) {
    if(i < c) {
      if(i == vec1.size()) {
        vec1.push_back(e.second);
      } else {
        vec1.at(i) = e.second;
      }
    } else {
      vec2.push_back(e.second);
      if(flags.count(e.second)){
        flag = true;
      }
    }
  }

I am new to C coming from python and R. Therefore, I assume that this can be simplified quite a bit (with iterators?). What can I do to improve the code here?

CodePudding user response：

Your code must increment i at the end of each loop for it to work.

If you can use c 20 and its ranges, I would probably rewrite it completely, to something like:

using namespace std::views; // for simplicity here
    
std::ranges::copy(m | take(c) | values, vec1.begin());
std::ranges::copy(m | drop(c) | values, std::back_inserter(vec2));
flag = std::ranges::any_of(vec2, [&flags](int i){return flags.contains(i);});

The beauty of this, is that it matches your requirements much better.

• The first lines does: "I want to copy as many values of m (in order) into vec1 (overwriting previous values) without exceeding the capacity c."
• The second line does: "If any values remain I want to push those values to the end of vec2."
• The third line does: "For each of these, values I want to check if they are in flags. If they are, I'd like to set flag to true."

CodePudding user response：

Building on the comments of @PaulMcKenzie and the answers provided by @Nelfeal and @cptFracassa, this is what I ended up with.

size_t new_size = std::min(vec1.capacity(), m.size());
vec1.resize(new_size);

std::transform(m.begin(),
               std::next(m.begin(), new_size),
               vec1.begin(),
               [](std::pair<int, int> p) { return p.second; });
std::transform(std::next(m.begin(), new_size),
               m.end(),
               std::back_inserter(vec2),
               [flags, &flag](std::pair<int, int> p) { 
                   if(flags.count(p.second)) {
                       flag = true;
                   }
                   return p.second;
               });

CodePudding user response：

In the first part, instead of doing either push_back or assignment to at, you can just clear the vector and push_back everything. clear does not change the capacity.

Your loop is doing two different things, one after the other (and by the way, I assume you forgot to increment i). You should split it into two loops.

With all that, your code becomes:

vec1.clear();
auto it = m.begin();
for (int i = 0; i < c;   i) {
    vec1.push_back(it->second);
      it;
}
while (it != m.end()) {
    vec2.push_back(it->second);
    if(flags.count(it->second)){
        flag = true;
    }
      it;
}

At this point, you can also use standard algorithms (std::copy, std::transform as mentioned in the comments).