3

Basically, I have code that looks like this. It's a typedef and a set of methods that revolve around that typedef.

class foo {
    // Foo stuff...

    // Registration Stuff
    private:
        typedef std::map<std::string, std::string> MTRegistrationParams;

        void DispatchRegistrationMessage (MTRegistrationParams const &, std::string destination_id);
        static std::string BuildRegistrationMessage (MTRegistrationParams const &, std::string message_id); // Only called by DispatchRegistrationMessage

    // more Foo stuff...
};

I am thinking it might be clearer to just make a little mini-class, to help logically separate those methods from Foo's structure.

class foo {
    // Foo stuff...

    // Registration Stuff
    private:
        struct MTRegistrationParams : public std::map<std::string, std::string> {
            void DispatchRegistrationmessage (foo &, std::string const & destination_id);
            private:
                std::string BuildRegistrationMessage (std::string const & message_id);
    };

    // more Foo stuff...
};

The thing is... I don't usually make derived classes unless for the sake of inheriting methods, and deriving from a standard container seems REALLY weird.

Am I being overly concerned, or is deriving from standard containers just to decorate them with methods genuinely bad practice?

  • 3
    I don't see any problems with the first approach. It is pretty clear and simple. Don't overcomplicate things for nothing. – glampert Sep 12 '14 at 2:07
  • 1
    @glampert, I don't think it's for nothing. When you have functions in a class that deal exclusively (or close) with methods/state/data of another class, that's a clear indication to refactor (i.e. DispatchRegistrationMessage and BuildRegistrationMessage receive a map as a parameter, because that's what they operate on; they should probably be in the map class - in this case, "the map class" means the extracted/refactored MTRegistrationParams) – utnapistim Sep 12 '14 at 8:56
3

Deriving from a C++ standard container should look really weird, because they were never designed to be used as a base class.

The biggest source of problems is if your 'extended' container needs to override behavior of the underlying container (including the destructor!), or if your 'extended' container has additional constraints that don't exist for the underlying standard container.
When passing such an 'extended' container as a pointer/reference to a function that expects a standard container, that function can inadvertently break the invariants of the 'extended' container by calling the wrong functions of the base class. And if you happen to destruct an 'extended' container through a pointer to its base class, you land yourself in undefined behavior.

In the example of this question, the chances for such incorrect behavior are next to nil, but if you start using this pattern, it is easy to land in situations where it does matter. For that reason, it is strongly recommended not to derive from the standard containers.

Also, the DispatchRegistrationMessage method isn't that much easier to use, because now you need to pass in a Foo object and it is very inconvenient to use overloaded operators (like operator[] of std::map) on this.

3

Am I being overly concerned, or is deriving from standard containers just to decorate them with methods genuinely bad practice?

No, you are right to be concerned. The std containers were not meant to be inherited, only encapsulated (and inheriting from them should be UB).

I think your final solution should be along these lines:

class MTRegistrationParams {
    std::map<std::string, std::string> data; // encapsulated
public:
    void DispatchRegistrationmessage (std::string const & destination_id);

    decltype(data.begin()) begin() { return data.begin(); }
    decltype(data.end()) end() { return data.end(); }
    decltype(data.size()) size() { return data.size(); }
    // forward ano other map methods you need, here
private:
    std::string BuildRegistrationMessage (std::string const & message_id);
};

class foo {
    // Foo stuff...

    // Registration Stuff
private:
    typedef MTRegistrationParams params;

    // more Foo stuff...
};
  • 1
    Any reference (e.g. to some precise part of the C+11 standard) about inheriting from standard containers is undefined behavior. I know it is bad practice, but why UB? – Basile Starynkevitch Sep 12 '14 at 10:51
  • My kingdom for “delegation” to be added to C++ for just these situations. – Bill Door Sep 12 '14 at 15:24
  • 1
    @BasileStarynkevitch, since the map destructor is not virtual, it will not be called, unless you call it explicitly. This means that any objects inside the map will not get destroyed, and that can mean anything from a memory leak, to a deadlock in the application (depending on what those destructors needed to do). – utnapistim Sep 12 '14 at 15:56
  • 3
    @glampert nope, the latter is not UB, since the static type of the pointer is the right one, so ~MTRegistrationParams is called. However, this would lead to UB: std::map<std::string, std::string>* pMap = new MTRegistrationParams(); delete pMap; – Arne Mertz Sep 18 '14 at 12:08
  • 2
    Standard reference for my comment: C++11, [expr.delete], §5.3.5,3: "[...] if the static type of the object to be deleted is different from its dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the static type shall have a virtual destructor or the behavior is undefined." (emphasis mine) – Arne Mertz Sep 18 '14 at 12:15
-1

it is fine. Because foo class is not associated logicaly with MTRegistrationParams. its is better to provide inner class. So if user want to use foo only then no need to expose MTRegistrationParams' functionalities. like iterator for list.

  • 2
    You missed the point of the question. It is fine to create inner class but it is not fine to derive it from std::map. – Jan Hudec Sep 12 '14 at 8:12
  • @ Jan Hudec : why is there a problem to derive inner class from std::map. is it because of template ? please explain me to improve my knowledge – Jai Sep 15 '14 at 13:06
  • Collections don't have any virtual methods, so you don't get anything and you risk breaking the subclass invariants by direct calls to the base class methods. It's better to delegate. – Jan Hudec Sep 15 '14 at 16:50

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.