Pointers to polymorphic classes saved in vector that reference data in a list as source

Question

I have a list called data with strings in it. I have a class String that derives from class Item. A class Scene to which I can add Items that are put in a vector. Scene has a print function to show the current Items.

If I change an element in the data list it also changes what it prints in the Scene.print().

When I remove an item from a Scene with function del it also removes the element in data (list).

Is this a good way to do it am I missing something? The code is actually for a game where I want to add items with coordinates in a scene. Each item has its own function to draw itself etc. and when I remove an item from the scene the data it relates to should be removed too.

#include <iostream>
#include <string>
#include <list>
#include <vector>
using namespace std;
list<string> data;
class Item {
public:
 Item() {
 }
 virtual void print() = 0;
};
class String : public Item {
public:
 String(string &s) {
 _s = &s;
 }
 virtual void print() override {
 cout << *_s << endl;
 }
 string *_s;
 string ret() {
 return *_s;
 }
};
class Scene {
public:
 void add(shared_ptr<Item> item) {
 _items.push_back(item);
 }
 void del(int n) {
 shared_ptr<Item> pa = _items[n];
 shared_ptr<String>pb = dynamic_pointer_cast<String>(pa);
 if (pb) {
 data.remove(pb->ret());
 }
 _items.erase(_items.begin()+n);
 }
 void print() {
 for(int i = 0; i < _items.size(); i++) {
 _items[i]->print();
 }
 }
 vector<shared_ptr<Item>> _items;
};
void print (list<string> l) {
 for (std::list<string>::iterator it = l.begin(); it != l.end(); ++it) {
 cout << "data: " << *it << endl;
 }
}
int main() {
 Scene s;
 data.push_back("first");
 data.push_back("seconed");
 data.push_back("third");
 data.push_back("fourth");
 std::list<std::string>::iterator it = data.begin();
 for (std::list<string>::iterator it = data.begin(); it != data.end(); ++it) {
 s.add(make_shared<String>(*it));
 }
 s.print();
 *it++;
 *it = "$$$";
 s.print();
 print(data);
 s.del(1);
 s.del(1);
 s.print();
 print(data);
 return 0;
}
```

Answer 1

Avoid using namespace std

While it is nice to save some typing, using namespace std should be avoided. See this post for more details: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

Have class Scene take ownership of the list

At the moment, the list data lives outside class Scene. This means other code can modify the list without Scene knowing about it. If you only want to allow items being added via Scene's add() and del() member functions, then make the list a private member variable. But in fact you already have a copy of the list in Scene, named _items. So make that the sole container of strings.

If you really need to have a std::vector<shared_ptr<Item>>, and a std::list<std::string>, then maybe they can be both member variables of class Scene?

Add virtual destructors to polymorphic classes

Whenever you inherit from a base class, you want the right destructor to be called. Therefore, especially if you have any non-trivial member variables, you need to declare a virtual destructor, even if it's empty:

class Item {
 public:
 Item() {}
 virtual ~Item() {}
 ...
};
class String: public Item {
 public:
 String(...) {...}
 virtual ~String() override {}
};

Consider:

Item *item = new String("foo");
delete item;

Without the virtual destructor, the base classes destructor would be called, which doesn't clean up String's member variable _s.

Consider making some member variables private

If a member variable should only ever be modified via a member function, then you should make that variable private. If other code needs to access it anyway, but only for reading, then add a function to get a const pointer or reference to the data. For example, in Scene, you don't want something to remove items directly from _items, because it would bypass the removal of strings from data. So make it private and add const access to it:

class Scene {
 std::vector<std::shared_ptr<Item>> _items;
public:
 ...
 const std::vector<std::shared_ptr<Item>> &get_items() const {
 return _items;
 }
}

Maybe the same can be applied to String as well.

Avoid reinventing the wheel

Perhaps there is a reason for it, but if I just look at the code you posted, I wonder why you create your own class String. If Scene is just a container for strings, then why not have it have std::vector<std::string> _items?

Use auto where appropriate

You can avoid repeating types by using auto in several places. For example, in del() you could write:

auto pa = items[n];
auto pb = dynamic_pointer_cast<String>(pa);

Use range-based for-loops

Whenever you are iterating over the items in a container, use range-based for loops. They are easier to write, and there is less chance of errors. For example:

for (auto item: _items) {
 item->print();
}

Consider using if-statements with initializers

C++17 made if-statements a bit more powerful. For example, especially when doing dynamic-casts, you can write shorter code:

if (auto pb = dynamic_pointer_cast<String>(pa); pb) {
 data.remove(pb->ret());
}

Prevent out-of-bounds access

In Scene::del(), you don't check whether the supplied parameter n is smaller than the size of _items. If this function is called with a too large value, this will result in an out-of-bounds access, which, in the best case, causes a segmentation fault, but in the worst case results in weird behavior that is hard to debug.

If it's something that could happen due to wrong input to the program, you should add a run-time check, and for example throw an exception if n is too large. If the only time n would be too large is due to a programming error somewhere else in the code, then consider adding an assert() statement.

Avoid using std::endl

Use \n instead. See this post for the rationale: https://stackoverflow.com/questions/213907/c-stdendl-vs-n

Consider overloading operator<<()

Instead of writing a print() function that always prints to std::cout, you could rewrite this to an overload of operator<<(), so that you could print your objects to any output stream. There is a slight problem when having derived classes though, the solution is to modify your print() functions to take an output stream object as a parameter, and then only overload operator<<() in the base class:

class Item {
public:
 ...
 virtual void print(std::ostream &out) const = 0;
 friend std::ostream& operator<<(std::ostream& out, const Item &item) {
 item.print(out);
 }
};
class String: public Item {
public:
 ...
 virtual void print(std::ostream &out = std::cout) const override {
 out << *_s << '\n';
 }
};

Now you can write:

String s;
...
std::cout << s;

I recommend you don't print the newline character in your print() functions though, so code that wants to print items can decide itself whether they should be on separate lines or not.

Make member functions that don't modify anything const

As I already did in the above example, functions that don't change any of the member variables should be made const. This will allow the compiler to perform more optimizations, and will cause it to generate errors when you accidentily do change something in const functions.

Another thing to consider is that if you have a const variable or const reference to a variable, then only member functions of that variable that are marked const can actually be called.

Don't move an iterator past the end

In your main(), you iterator over data. After the for loop, it is equal to data.end(). Then you write:

*it++;

Depending on the implementation of std::list<>, this might cause the iterator to become invalid, and either that statement itself will cause some error, or the subsequent *it = "$$$" will.

• kngkrl: wow thx alot i will try to incorporate your suggestions.