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;
}
\$\endgroup\$
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 (-2³¹ 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.

| improve this answer | |
\$\endgroup\$
  • \$\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\$ – István Simon Apr 4 '18 at 0:54
  • \$\begingroup\$ const_cast from what to what? What are you mean for threat to? \$\endgroup\$ – István Simon Apr 4 '18 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\$ – bipll Apr 4 '18 at 0:59
  • \$\begingroup\$ Yes in this way anybody can overwrite the value. Thanks. \$\endgroup\$ – István Simon Apr 4 '18 at 1:06
  • \$\begingroup\$ @IstvánSimon I have rolled back Rev 3 → 2. Please see What to do when someone answers. \$\endgroup\$ – 200_success Apr 4 '18 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 ☹.

| improve this answer | |
\$\endgroup\$
  • \$\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\$ – István Simon Apr 5 '18 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\$ – István Simon Apr 5 '18 at 7:28
  • 1
    \$\begingroup\$ You are right, your solution is much better for what the example try to present. \$\endgroup\$ – István Simon Apr 5 '18 at 7:33

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.