-1
\$\begingroup\$

I have already got the answer for my previous question, and I decided to use std::vector<int> instead of int *. I have written the following code. Am I use shared_ptr properly now?

class MyClass
{
public:
    MyClass(unsigned size, std::string name) {
        _size = size;
        _name = name;
        _arr = std::make_shared<std::vector<int> >(size, 0);
    }

    MyClass(const MyClass& other) {
        _arr = other._arr;
        _size = other._size;
    }

    MyClass& operator=(const MyClass& other) {
        _arr = other._arr;
        _size = other._size;
    }

    void setArrAt(unsigned index, int value) {
        if (index < _size)
            (*_arr.get())[index] = value;
    }

    void setArr(std::shared_ptr<std::vector<int> > &arr) {
        _arr = arr;
    }

    std::shared_ptr<std::vector<int> >& getArr() {
        return _arr;
    }

    void print() {
        std::cout << _name << " : ";
        for (int i = 0; i < _size; i++)
            std::cout << (*_arr.get())[i] << " ";
        std::cout << std::endl;
    }

    ~MyClass() { }

private:
    std::shared_ptr< std::vector<int> > _arr;
    unsigned _size;
    std::string _name;
};

int main() {
    MyClass alice(2, "Alice");
    alice.setArrAt(0, 10);
    alice.setArrAt(1, 20);
    MyClass bob(2, "Bob");
    // some code
    bob.setArr(alice.getArr());
    alice.print(); // "Alice : 10 20"
    bob.print(); // "Bob : 10 20"

    bob.setArrAt(0, 100);
    alice.print(); // "Alice : 100 20"
    bob.print(); // "Bob : 100 20"
    return 0;
}

P.S. I use _ in names of variables only for clarity.

\$\endgroup\$

closed as off-topic by Mast, 200_success Dec 3 '16 at 14:51

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I really didn't see any benefit of using shared_ptr in here rather than unique_ptr? Also for clarity I would suggest to put m as a prefix for the member variables rather than putting an underscore. \$\endgroup\$ – Onur A. Dec 3 '16 at 14:09
  • 1
    \$\begingroup\$ @OnurA. I actually don't use underscores in my code, it's only for this example. \$\endgroup\$ – AnatoliySultanov Dec 3 '16 at 14:14
  • \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Mast Dec 3 '16 at 14:18
1
\$\begingroup\$

std::vector already takes care of managing the memory of a dynamically growing array. Storing a pointer to the vector only complicates stuff and reduces performance due to an extra indirection when accessing the elements of a vector.

Other than that, I would only expose shared pointers via getters/setters when wanting to actually share the lifetime of resources. Returning a (const) reference to the object is much cleaner if the calling code only wants to access it, not control it's lifetime.

Why do you also store _size? The vector already stores it's current size.

What happens to _name if you copy the object? It's been left out of the copy constructor & assignment operators. These two also duplicate code which can be avoided with the copy-and-swap idiom.

\$\endgroup\$
  • \$\begingroup\$ If alice and bob have identical _array, this vector<int> will be deleted twice: in alice.~MyClass() and bob.~MyClass(), and I will get segfault. Am I right? \$\endgroup\$ – AnatoliySultanov Dec 3 '16 at 14:27
  • \$\begingroup\$ No, by storing the std::vector as a member of the class (not as a (smart) pointer), each instance of the class has a different vector. \$\endgroup\$ – D. Jurcau Dec 3 '16 at 14:31
  • \$\begingroup\$ Yes, but I need identical vectors. Also, MyClass is not my implementation of vector. \$\endgroup\$ – AnatoliySultanov Dec 3 '16 at 14:34
  • 2
    \$\begingroup\$ @AnatoliySultanov, describe the situation in the post itself then. Currently people think that it is hypothetical code. Also, try to rename MyClass into something considerable, that will give more context to your post and make it actually reviewable. \$\endgroup\$ – Incomputable Dec 3 '16 at 14:37

Not the answer you're looking for? Browse other questions tagged or ask your own question.