4
\$\begingroup\$

I want a function Select that takes a vector<int> and a condition (such as being even, being divisible by three, being larger than 4, or any logical combination thereof), and returns another vector<int> composed of the members of the original vector satisfying the condition.
For example Select(v, IsEven) should return the even members of v.
Select(v, !IsEven && LessThan5) should return odd members of v that are less than 5.
And for the final example Select(v, IsEven || !LessThan5) should return elements of v that are either even or greater or equal to 5.

It is important that the conditions are represented by names rather than logical expressions. In other words I prefer Select(v, IsEven) to Select(v, [](int x)->bool{return x%2==0;}). This is because I expect to use the same properties multiple times and would rather not make mistakes copying and pasting. I also think it makes the code more readable.

I could write boolean functions for the basic conditions, and for the composite statements make use of lambdas i.e. Select(v, [](int x){ return !IsEven(x) && LessThan5(x);}). But again I think this still looks ugly, and I expect to use these Select functions repeatedly through the code so I would prefer to type less.

I came up with the solution of using function objects derived from a common base class as seen below.

#include <iostream>
#include <vector>

using namespace std;

//Condition Base and Logical Classes
class NotCondition;
class AndCondition;
class OrCondition;

class BaseCondition{
public:
  BaseCondition(){}
  virtual bool operator () (int x) const = 0;
  friend const NotCondition operator !(const BaseCondition& cond);
  friend const AndCondition operator &&(const BaseCondition& left, const BaseCondition& right);
  friend const OrCondition operator ||(const BaseCondition& left, const BaseCondition& right);
};

class NotCondition : public BaseCondition{
public:
  NotCondition(const BaseCondition& cond) : BaseCondition(), cond_{cond} {}
  bool operator () (int x) const override {return !cond_(x);} 

private:
  const BaseCondition& cond_;
};

const NotCondition operator !(const BaseCondition& cond){
  return NotCondition(cond);
}

class AndCondition : public BaseCondition{
public:
  AndCondition(const BaseCondition& left, const BaseCondition& right) : BaseCondition(), left_{left}, right_{right} {}
  bool operator () (int x) const override {return left_(x) && right_(x);}

private:
  const BaseCondition& left_;
  const BaseCondition& right_;
};

const AndCondition operator &&(const BaseCondition& left, const BaseCondition& right){
  return AndCondition(left, right);
}

class OrCondition : public BaseCondition{
public:
  OrCondition(const BaseCondition& left, const BaseCondition& right) : BaseCondition(), left_{left}, right_{right} {}
  bool operator () (int x) const override {return left_(x) || right_(x);}

private:
  const BaseCondition& left_;
  const BaseCondition& right_;
};

const OrCondition operator ||(const BaseCondition& left, const BaseCondition& right){
  return OrCondition(left, right);
}


// Condition Derived Classes

class IsEven : public BaseCondition{
public:
  bool operator ()(int x) const override {return x % 2 ==0;}
};

struct IsGreaterThan : public BaseCondition{
public:
  IsGreaterThan(int c) : compare_{c}{}
  bool operator ()(int x) const override {return x > compare_;}
private:
  int compare_;
};

struct IsLessThan : public BaseCondition{
public:
  IsLessThan(int c) : compare_{c}{}
  bool operator ()(int x) const override {return x < compare_;}
private:
  int compare_;
};

vector<int> Select (const vector<int>& vect, const BaseCondition& condition){
  vector<int> result;
  for(const auto& item: vect)
    if(condition(item))
      result.push_back(item);
  return result;
}

//just so we can cout << vectors.
template<typename T>
ostream& operator << (ostream& strm, const vector<T>& v){
  for(const auto& item: v)
    cout << item << " ";
  return strm;
}

//macro to print out the test Conditions and their values
#define TEST(x) cout << #x" = " << x << endl; 

int main(){
  vector<int> test = {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};

  cout << "test = " << test << endl;

  TEST(Select(test, IsEven()));
  TEST(Select(test, IsGreaterThan(5) && IsLessThan(10)));
  TEST(Select(test, IsLessThan(5) || IsGreaterThan(10)));
  TEST(Select(test, !IsEven()));
  TEST(Select(test, !!IsEven()));
  TEST(Select(test, !IsGreaterThan(10) && !IsLessThan(5)));
  TEST(Select(test, !IsGreaterThan(10) && !IsLessThan(5) || IsEven()));


  return 0;
}

This program's output is as below:

test = 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 
Select(test, IsEven()) = 2 4 6 8 10 12 14 
Select(test, IsGreaterThan(5) && IsLessThan(10)) = 6 7 8 9 
Select(test, IsLessThan(5) || IsGreaterThan(10)) = 1 2 3 4 11 12 13 14 15 
Select(test, !IsEven()) = 1 3 5 7 9 11 13 15 
Select(test, !!IsEven()) = 2 4 6 8 10 12 14 
Select(test, !IsGreaterThan(10) && !IsLessThan(5)) = 5 6 7 8 9 10 
Select(test, !IsGreaterThan(10) && !IsLessThan(5) || IsEven()) = 2 4 5 6 7 8 9 10 12 14 

So it seems to work. My questions are:
1. Are there any dormant bugs in this code that may show up later on?
2. How would you improve this code for readability, efficiency and/or safety?

\$\endgroup\$
  • 1
    \$\begingroup\$ I would say that using namespace std; is unsafe. \$\endgroup\$ – Null Feb 25 '16 at 14:47
  • 1
    \$\begingroup\$ Agreed, though normally I would seperate this code into three files: Condition.h, Condition.cpp, main.cpp and not use namespace std in the header file. \$\endgroup\$ – Aydin Gerek Feb 25 '16 at 16:30
  • 2
    \$\begingroup\$ It's not recommended to use use namespace... in source file: it can be some conflict with different using namespace. Prefer using std::vector; or using myVector = std::vector which explicit what you use in namespace \$\endgroup\$ – Garf365 Feb 25 '16 at 21:53
6
\$\begingroup\$

Personally, I found your implementation to be really readable. I have no opinion about efficiency.

But, for me, you reimplement std::copy_if:

std::vector<int> input{1, 2, 3, 4 5, ... };
std::vector<int> result(input.size());

auto it = std::copy_if(input.begin(), 
                  input.end(),
                  result.begin(),
                  [](int i)->bool { return (i%2)==0; });
result.resize(std::distance(result.begin(), it));

You can encapsulate this one as you want to obtain the readability you want. Using this is more scalable, because you just have to rewrite condition in builtin functionality. You can also call function. Less code, less dependency, native bool operation, ...

constexpr bool isEven(int i) { return (i%2) == 0; }

template<typename UnaryPredicate>
std::vector<int> Select(std::vector<int> v, UnaryPredicate p)
{
    std::vector<int> result(v.size());
    auto it = std::copy_if(v.begin(),
                  v.end(),
                  result.begin(),
                  p);
    result.resize(std::distance(result.begin(), it));
    return result;
}

Select(input, [](int i)->bool { return isEven(i) && i > 5 });

Or, without template

constexpr bool isEven(int i) { return (i%2) == 0; }

std::vector<int> Select(std::vector<int> v, std::function<bool(int)> p)
{
    std::vector<int> result(v.size());
    auto it = std::copy_if(v.begin(),
                  v.end(),
                  result.begin(),
                  p);
    result.resize(std::distance(result.begin(), it));
    return result;
}

Select(input, [](int i)->bool { return isEven(i) && i > 5 });

For safety, I prefer to use STL or Boost or common lib than reimplement by myself.

\$\endgroup\$
  • \$\begingroup\$ Excellent advice on using already existing libraries, thank you. \$\endgroup\$ – Aydin Gerek Feb 25 '16 at 16:32

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.