0
\$\begingroup\$

Please review my solution for the following problem. I am interested in:

  • What do you think of the design
  • Improvement tips
  • Usability
  • Efficiency

Problem: Need to be store policy-based objects with different template parameters in one container, and reach the common interface.

Solution: C++ Shell

#include <exception>
#include <iostream>
#include <memory>
#include <string>
#include <vector>
class SinglePolicy
{
public:
 SinglePolicy() = default;
 void push(int data)
 {
 std::cout << "set to SinglePolicy\n";
 buffer_ = data;
 }
 int pop()
 {
 std::cout << "return from SinglePolicy\n";
 int data = buffer_;
 buffer_ = -1;
 return data;
 }
protected:
 int buffer_;
};
class VectorPolicy
{
public:
 VectorPolicy() = default;
 virtual void push(int data)
 {
 std::cout << "emplace_back to VectorPolicy\n";
 buffer_.emplace_back(data);
 }
 virtual int pop()
 {
 std::cout << "pop front from VectorPolicy\n";
 if(buffer_.empty())
 return -1;
 int data = buffer_[0];
 buffer_.erase(buffer_.begin());
 return data;
 }
protected:
 std::vector<int> buffer_;
};
template<class BufferPolicy>
class PolicyBasedBuffer
 : public BufferPolicy
{
public:
 PolicyBasedBuffer()
 : BufferPolicy()
 {}
};
typedef std::shared_ptr<PolicyBasedBuffer<SinglePolicy>> SinglePolicyPtr;
typedef std::shared_ptr<PolicyBasedBuffer<VectorPolicy>> VectorPolicyPtr;
class PolicyInterface
{
public:
 PolicyInterface(const std::function<void(int)>& pushArg, const std::function<int(void)>& popArg)
 : push(pushArg)
 , pop(popArg)
 {}
 const std::function<void(int)> push;
 const std::function<int(void)> pop;
};
typedef std::shared_ptr<PolicyInterface> PolicyInterfacePtr;
PolicyInterfacePtr saveToPolicyInterface(const SinglePolicyPtr& aPolicy)
{
 return std::make_shared<PolicyInterface>([aPolicy](int data) { aPolicy->push(data); }, [aPolicy]() { return aPolicy->pop(); });
}
PolicyInterfacePtr saveToPolicyInterface(const VectorPolicyPtr& aPolicy)
{
 return std::make_shared<PolicyInterface>([aPolicy](int data) { aPolicy->push(data); }, [aPolicy]() { return aPolicy->pop(); });
}
int main()
{
 auto singleBuffer = std::make_shared<PolicyBasedBuffer<SinglePolicy>>();
 auto vectorBuffer = std::make_shared<PolicyBasedBuffer<VectorPolicy>>();
 const int seven = 7;
 std::cout << "PolicyBasedBuffer test:\n";
 singleBuffer->push(seven);
 if(seven != singleBuffer->pop())
 throw std::runtime_error("SinglePolicy :: wrong return data");
 vectorBuffer->push(seven);
 if(seven != vectorBuffer->pop())
 throw std::runtime_error("VectorPolicy :: wrong return data");
 std::vector<PolicyInterfacePtr> policy;
 policy.push_back(saveToPolicyInterface(std::make_shared<PolicyBasedBuffer<SinglePolicy>>()));
 policy.push_back(saveToPolicyInterface(std::make_shared<PolicyBasedBuffer<VectorPolicy>>()));
 std::cout << "\nPolicyInterface test:\n";
 for(size_t i=0; i < policy.size(); ++i)
 {
 policy[i]->push(seven);
 if(seven != policy[i]->pop())
 throw std::runtime_error("policyInterface :: wrong return data");
 }
 std::cout << std::flush;
}
asked Apr 3, 2018 at 22:38
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Few minor notes.

  1. You have deliberately assigned -1 to represent undef, correct? The question whether you'll ever need to store -1 aside, you should probably define a named constant to get rid of magic number (it also helps when you decide to accept -1s). BTW, the minimal int (-231 or so) is maybe a better alternative.

  2. Every constructor and factory function accepts arguments by const ref. While acceptable for shared_ptr args, it makes sense to accept others (such as closures) by value and then move them inside the object's members.

  3. const_cast is a threat to PolicyInterface's public push and pop methods.

  4. VectorPolicy pushes elements to its right end but pops them on the left. If it should work as a LILO queue, std::deque is a better alternative to std::vector. Otherwise, VectorPolicy should follow stack (LIFO) idiom, and elements popped at the end.

answered Apr 4, 2018 at 0:35
\$\endgroup\$
5
  • \$\begingroup\$ "Need to be store policy-based objects with different template parameters in one container." That means the VectorPolicy and SinglePolicy and the PolicyBasedBuffer only example for usage. \$\endgroup\$ Commented Apr 4, 2018 at 0:54
  • \$\begingroup\$ const_cast from what to what? What are you mean for threat to? \$\endgroup\$ Commented Apr 4, 2018 at 0:56
  • 1
    \$\begingroup\$ One can write something like const_cast<std::function<void(int)> &>(buffer.push) = [](int){ std::cout << "Ignored an int" << std::endl; }; and the object is no more consistent or safe, \$\endgroup\$ Commented Apr 4, 2018 at 0:59
  • \$\begingroup\$ Yes in this way anybody can overwrite the value. Thanks. \$\endgroup\$ Commented Apr 4, 2018 at 1:06
  • \$\begingroup\$ @IstvánSimon I have rolled back Rev 3 → 2. Please see What to do when someone answers . \$\endgroup\$ Commented Apr 4, 2018 at 1:39
2
\$\begingroup\$

I’m having trouble following this code.

You have classes SinglePolicy and VectorPolicy that look like they should be polymorphic versions of an abstract policy because they have the same push and pop functions which are declared virtual. But they are not derived from anything.

Then, you have a template PolicyBasedBuffer which as far as I can tell doesn’t do anything at all. It derives from one of the above classes, but then adds no functionality or members at all. So why?

Then you have PolyInterface which holds pointers to push and pop functions, with free functions to populate with the push/pop from the original Policy classes.

That’s what the base class would have done automatically.

I think what you want is:

class PolicyInterface {
public:
 virtual void push(int data) = 0;
 virtual int pop() = 0;
};
class VectorPolicy : public PolicyInterface {
public:
 void push(int data) override 
 {
 // ⋯ your implementation here
 }
 ⋮

and likewise for the ScalarPolicy — derive it from PolicyInterface.

Now, you can have:

std::vector<unique_ptr<PolicyInterface>> policies;
policies.push_back(make_shared<VectorPolicy>());
policies.push_back(make_shared<ScalarPolicy>());
 ⋮
policies[i]->push(seven);

You mentioned policy-based objects with different template parameters in your problem statement. I don’t see any need for a template at all here, but perhaps the concrete example code simply doesn’t show the need?

The overall idea here is still the right thing. The derived class can be a template, which inherits from the interface. Stated like that, it sounds a lot like what your original code did! But you were deriving a meaningless class from the needed class, not deriving the needed class (which might happen to be templated) from an interface. Since you had virtual functions in the policy classes, I think you were confused and this is what you were trying to do.

template <typename T, size_t N>
class FlexPolicy : PolicyInterface {
 ⋮
};
policies.push_back( make_unique<FlexPolicy<string,5>> );

Misc tip: use the std::stack container adaptor instead of a plain vector.

I like your use of the C++shell web site for holding the code. Too bad it doesn’t have C++17 ☹.

answered Apr 5, 2018 at 3:57
\$\endgroup\$
3
  • \$\begingroup\$ I get it, I didn't take enough time to the example for present correctly the problem. I will create a new question a much better example. PolicyInterface's concept would be the subject of the question where store function pointers in the class. bipll pointed to a mistake. \$\endgroup\$ Commented Apr 5, 2018 at 7:14
  • \$\begingroup\$ std::stack is a FILO solution, but I use hear a FIFO solution. Maybe the std::deque can be better than std::vector, but with small element size the std::vector is faster. \$\endgroup\$ Commented Apr 5, 2018 at 7:28
  • 1
    \$\begingroup\$ You are right, your solution is much better for what the example try to present. \$\endgroup\$ Commented Apr 5, 2018 at 7:33

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.