4
\$\begingroup\$

I would like your comments on my code in C++ (C++98 + Boost)

I have written some code to use the functors with the boost::range::for_each algorithm. My main problem was to add some "external" parameters to the functor that are not in the vector on which I am using a for_each loop.

In main() we have:

std::vector<Person> vec;
vec.reserve(MAX_SIZE);
std::map<std::string, int> result;
FillPersons(vec);
boost::range::for_each(vec,Treatment(&result));

The functor is:

struct Treatment {
    Treatment(std::map<std::string, int> *p_result):result_(p_result){}
    void operator ()(Person &person)
    {
        if (NULL != result_)
        {
            int age = person.getAge();

            if (age < AGE_LOW_LEVEL)
            {
                age += 2;
            }
            else if ((age >= AGE_LOW_LEVEL) && (age < AGE_HIGH_LEVEL))
            {
                age += 1;    
            }
            else if (age >= AGE_HIGH_LEVEL)
            {
                age -= 5;    
            }
            result_->insert(std::pair<std::string, int> (person.getName(), age));
        }
    }
    private:
    std::map<std::string, int> *result_;
    Treatment(const std::map<std::string, int> &);
    Treatment& operator=(const Treatment&);
};
  1. Is there any other way to do that better or is it good?
  2. How would it be using lambdas?
  3. How would it be in C++11?

For your information, the Treatment function is naive because my focus was only on adding external parameters to the functors.

\$\endgroup\$

migrated from stackoverflow.com Jun 16 '14 at 14:48

This question came from our site for professional and enthusiast programmers.

3
\$\begingroup\$

First of all I would not use boost::range::for_each . There is already standard algorithm std::for_each in C++.

Secondly I would not use a lambda-expression if the algorithm with the predicate is used more than one time.

And if it is used only one time I would use the range-based for statement. For example

for ( const Person &person : vec )
{
    int age = person.getAge();

    if ( age < AGE_LOW_LEVEL )
    {
        age += 2;
    }
    else if ( age < AGE_HIGH_LEVEL )
    {
        age += 1;   
    }
    else
    {
        age -= 5;   
    }

    result.insert( { person.getName(), age } );
}    
\$\endgroup\$
3
\$\begingroup\$

I would suggest to factor the code differently.

In fact, you're transforming collection 1 (Persons in vec) into a map of (name, rank) where rank is somehow calculated from the age of a person.

I'd start with that ageRank:

/*static*/ int ageRank(int age) {
    if (age < AGE_LOW_LEVEL)  return age + 2; 
    if (age < AGE_HIGH_LEVEL) return age + 1;   

    return age - 5;
}

There, that's simple enough. Now that transform:

boost::transform(
        vec, 
        std::inserter(result, result.end()), 
        make_pair_(phx::bind(&Person::getName, arg1), ageRank_(arg1))
    );

Now, how did that happen?

Well, I used a little bit of Boost Phoenix (which is the successor to the Boost Lambda/Boost Bind libraries). I defined lazy functions that are used: ageRank_ and make_pair_.

int ageRank(Person const& person) {
    return ageRank(person.getAge());
}

BOOST_PHOENIX_ADAPT_FUNCTION(int, ageRank_, ageRank, 1)
phx::function<boost::proto::functional::make_pair> make_pair_;

Not too much effort, right? Most of the stuff is just already there in Boost, now see it Live On Coliru.

int main() {
    std::vector<Person> vec;
    vec.push_back(Person("John",   12));
    vec.push_back(Person("Sara",   2));
    vec.push_back(Person("Chang",  73));
    vec.push_back(Person("Mbekip", 24));

    typedef std::map<std::string, int> RankMap;
    RankMap result;

    boost::transform(
            vec, 
            std::inserter(result, result.end()), 
            make_pair_(phx::bind(&Person::getName, arg1), ageRank_(arg1))
        );

    for(RankMap::const_iterator it=result.begin(); it!=result.end(); ++it)
    {
        std::cout << it->first << " -> " << it->second << "\n";
    }
}

Prints

Chang -> 68
John -> 13
Mbekip -> 25
Sara -> 4
\$\endgroup\$
1
\$\begingroup\$

With C++11 and lambda, it will be more straightthrough.

for_each(vec.begin(), vec.end(), [&result](Person& person){
    if (NULL != result)
    {
      int age = person.getAge();

      if (age < AGE_LOW_LEVEL)
      {
        age += 2;
      }
      else if ((age >= AGE_LOW_LEVEL) && (age < AGE_HIGH_LEVEL))
      {
        age += 1;   
      }
      else if (age >= AGE_HIGH_LEVEL)
      {
        age -= 5;   
      }
      result.insert(std::pair<std::string, int> (person.getName(), age));
    }
});
\$\endgroup\$
  • \$\begingroup\$ can we use boost::lambda because I cannot use C++11 ? \$\endgroup\$ – user3737114 Jun 13 '14 at 9:36
  • \$\begingroup\$ With C++11 or newer, it'd be more like for(auto person : vec) {....} \$\endgroup\$ – phresnel Jun 13 '14 at 10:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy