5

According to Is it bad practice to use Inheritance to associate methods with a basic container?, I know it is bad to inherit form std containers, mainly because std containers are not designed to be inherited.

However, I found a scenario that I don't know if it is reasonable to inherit a vector, for example, I have an object that the data needs to be loaded from json using rapidjson:

class School{
private:
    std::string name;
    int age;
    std::vector<Student> students;
    std::vector<Teacher> teachers;
public:
    void jsonToObj(rapidJson::Value& value){
        this->name=value["name"].GetString();

        this->age=value["age"].GetInt();

        this.students.clear();
        for(auto itr=value["students"].Begin();itr!=value["students"].End();++itr){
            Student student;
            student.jsonToObj(itr.GetObject());
            this->students.push_back(student);
        }

        this.teachers.clear();
        for(auto itr=value["teachers"].Begin();itr!=value["teachers"].End();++itr){
            Teacher teacher;
            teacher.jsonToObj(itr.GetObject());
            this->teachers.push_back(teacher);
        }
    }
}

I found I need to copy the long for loop for each vector currently:

this.students.clear();
for(auto itr=value["students"].Begin();itr!=value["students"].End();++itr){
    Student student;
    student.jsonToObj(itr.GetObject());
    this->students.push_back(student);
}

So I create a helper class to convert the array:

template<class T>
class VectorHelper<T>{
public:
    void jsonToObj(rapidJson::Value& value,std::vector<T>& vector){
        vector.clear();
        for(auto itr=value.Begin();itr!=value.End();++itr){
            T t;
            t.jsonToObj(itr.GetObject());
            vector.push_back(t);
        }
    }
}

So that I can use the VectorHelper in School as the following:

class School{
private:
    std::string name;
    int age;
    std::vector<Student> students;
    std::vector<Teacher> teachers;
public:
    void jsonToObj(rapidJson::Value& value){
        this->name=value["name"].GetString();

        this->age=value["age"].GetInt();

        VectorHelper<Student>::jsonToObj(value["students"],this->students);

        VectorHelper<Teacher>::jsonToObj(value["teachers"],this->teachers);
    }
}

The code above seems look better, however, I found it has still a disadvantage: it has bad readability because "this->students" appears at the right hand side, which I want the properties appear in left hand side, so I create a class inherit from vector:

template<class T>
class MyVector : public std::vector<T>{
public:
    void jsonToObj(rapidJson::Value& value){
        this->clear();
        for(auto itr=value.Begin();itr!=value.End();++itr){
            T t;
            t.jsonToObj(itr.GetObject());
            this->push_back(t);
        }
    }
}

Now the vector in School class becomes MyVector:

class School{
private:
    std::string name;
    int age;
    MyVector<Student> students;
    MyVector<Teacher> teachers;
public:
    void jsonToObj(rapidJson::Value& value){
        this->name=value["name"].GetString();

        this->age=value["age"].GetInt();

        this->students->jsonToObj(value["students"]);

        this->teachers->jsonToObj(value["teachers"]);
    }
}

which the vector also uses active record pattern (the jsonToObj method is inside the vector), and the class properties appear at the left of the line, which I think has better readability, for example: easier to see if any class properties missed.

So my question is, is the scenario above a reason to inherit from vector?

Improve this question
6
  • 5
    "So I create a helper class to convert the array:" - Why is this a class instead of just a free function? It looks like you learned programming in a strictly object-oriented language (Java, C#) and are stuck in some habits that mixed paradigm languages like C++ don't need. Commented Aug 28, 2024 at 7:40
  • students->jsonToObj looks misnamed. You are converting a json array to std::vector. Commented Aug 28, 2024 at 8:41
  • @SebastianRedl: you are correct, but using a free function does not change the gist of the question. Assuming jsonToObj a free function in a namespace VectorHelper, the call in stake would still look like VectorHelper::jsonToObj<Student>(value["students"],this->students); Commented Aug 28, 2024 at 8:57
  • 3
    Why not return a vector from your conversion function, i.e. template <typename T> std::vector<T> jsonToVector(rapidJson::Value&)? Then you can just write this->students = jsonToVector<Student>(value["students"]); and it looks just like the other two assignments above it. Commented Aug 28, 2024 at 21:28
  • +1 to @MilesBudnek comment. This is also a textbook place to use emplace_back rather than construct...push_back. And if returning a vector, use move semantics. Commented Aug 29, 2024 at 0:14

2 Answers 2

Reset to default
5

This has nothing to do with the ActiveRecord pattern, that's a red herring.

What you are trying to do here is (more or less) to implement the equivalent of a C# extension method in C++. Specifically, you want an extension method jsonToObj for the type std::vector<T>. Unfortunately, C++ does not have extension methods, and the gross consensus in the C++ community is that they are not needed. Their idiomatic way of extending existing types like std::vector is to use free functions and live with the fact that instead of

 students->jsonToObj( ...

one will have to write

 jsonToObj(students ...

See this SO question for a discussion about extension methods in C++:

IN 2014, Stroustrup and Sutter both made proposals for extending C++ in this direction, but AFAIK these were never accepted by the C++ standard board:

Using a derived class like MyVector for extending a type will technically work to some degree, as long as you can restrict the usage to an isolated context where no polymorphism will be required. However, as soon as you need more extension methods, maybe methods placed by someone else in a different derivation, or when you run into a situation where you need to apply jsonToObj to something you would have to cast to a MyVector first, then this will cause trouble sooner or later. It also gives a bad example for other cases. If you don't must, don't do it.

Disclaimer: I have derived from std::vector by myself in the past. This was for porting a custom vector class from some old 16 bit C++ code base to a modern C++ compiler with minimal changes to existing code, where we required the same code to compile

  • in an old 16 bit environment without any standard libs available, as well as

  • in a more modern environment, where we wanted to replace the custom vector class by std::vector, but with a backwards-compatible API.

We needed this setup for more than a year, and it served us well.

This was the most pragmatic solution we could come up with, and I am sure I am not going to hell for this - hence I don't think in terms of "good" and "bad" over such design decisions. Still I would recommend against such practices in new code.

Improve this answer
4

No, the active record pattern is not at all a justification for inheriting of standard containers (at least in C++).

But the fact that you mention it, suggests that separation of concerns could be strengthened in you design. Indeed, it is one concern to have a domain object responsible for loading and saving its state to persistent storage, and it is another concern to manage the persistence.

Here the persistence seems to be managed via serialisation. Just take a serialisation library, or keep up with the helper. It's ugly but it works well, is easy to maintain, and will not create accidental leaks at destruction.

But think also on step further: how would your design be able to deal with an RDBMS, where the two steps approach: prepare Json, /save json may not be transposed). You ain't got a need it, but just thinking about it 5 minutes shows that your persistence management is completely entangled in you domain model. This is a serious limitation

What you need is more an interchangeable strategy that controls saving/loading of objects rather than a personalised vector and the associated risks.

By the way: the C++ core guidelines explain that the destructor of a base class should either be public and virtual or protected and non-virtual. The reason is to protect from accidental UB. Unfortunately the standard containers all have public non virtual destructors. This is the main case against this inheritance. So it's not about taste, and going by the books, but about ensuring reliability. If you chose to go that way, you need to be extra careful.

Improve this answer
4
  • "shows that your persistence management is completely entangled in you domain model." - sure, but isn't that an issue of the active record pattern in general, regardless how it is implemented? Commented Aug 28, 2024 at 16:30
  • @DocBrown yes and no. Yes the AR has the drawback of having the data access logic in the domain logic. However, in the usual way it is done, the persistence operations are isolated in the crud logic. If I want to change db or serialization format, I look there and only there. moreover with some more abstraction you could inject some db object (or dynamically look for such object) to abstract low level db details. The design of the OP breaks the glass of toxic waste and contaminate the whole object. The vecors are now part of the access layer as well and what else... Commented Aug 28, 2024 at 19:01
  • ... you will even have to distinguish everytime you add a vector: is this relevant for persistance ? or is this some throw-away mutable that is temporary and internal to the objects. I would not like to have to maintain such a thing ... Commented Aug 28, 2024 at 19:03
  • 1
    I think this is all good advice what you wrote, the design is debatable. Still I would not put the bar so high for this kind of contrived demo example, which actually focusses on a different thing (when inheriting from a standard container may be justified) The fact of the missing virtual destructor is a good point which I missed (+1 for that, and to balance the unexplained downvote). Commented Aug 28, 2024 at 19:18

Your Answer

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.