HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++ smart pointers and classes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andsmartpointersclasses

Problem

I've started learning C++ recently and want from what I've gathered smart pointers are the way to go when storing stuff on the free store. I want to make sure I've got the basic hang of C++(11) correctly. I've implemented a very basic vector class (a mathematical vector) and would greatly appreciate if anyone would point out if I'm doing something wrong or that could be done better.

Vector.h

#ifndef VECTOR
#define VECTOR

#include 

class Vector {
private:
    int x;
    int y;
    int z;

public:
    Vector(int x, int y, int z);
    ~Vector();

    std::unique_ptr operator + (Vector* vector);

    int GetX() { return x; }
    int GetY() { return y; }
    int GetZ() { return z; }

    void SetX(int x) { this->x = x; }
    void SetY(int y) { this->y = y; }
    void SetZ(int z) { this->z = z; }
};

#endif


Vector.cpp

#include 
#include 
#include "Vector.h"

Vector::Vector(int x, int y, int z) : x(x), y(y), z(z) {
    std::cout  Vector::operator + (Vector* vector) {
    std::unique_ptr result = std::make_unique(0, 0, 0);

    result->SetX(this->x + vector->GetX());
    result->SetY(this->y + vector->GetY());
    result->SetZ(this->z + vector->GetZ());

    return result;
};


Usage

#include 
#include 
#include "Vector.h"

int main() {
    std::unique_ptr v1 = std::make_unique(1, 2, 3);
    std::unique_ptr v2 = std::make_unique(3, 2, 1);
    std::unique_ptr vr = *(v1.get()) + v2.get();
}


When using the overloaded operator (+) it looks a bit ugly but it's the way I found that worked when using unique_ptr. One thing I'm doubting if I'm doing "correctly" is returning a unique_ptr. Should I return a value instead and then create a unique_ptr from that instead?

Solution

I dislike that your class mixes functionality (a mathematical vector) with memory management. A class should only do one of those. If I prefer shared_ptr I have to rewrite your whole Vector class.

There is actually a thing in C++ that is better than smart pointers, which is value semantics. People rarely screw up the lifetime management of an int, it just works naturally. When you do tricky lifetime management try to mimic the way int does it.

The operator + should be const (as well as GetX, GetY and GetZ). It should also work on Vectors and not only on pointers. The standard way to add Vectors is this:

Vector operator +(const Vector &lhs, const Vector &rhs){
    return Vector(lhs.GetX() + rhs.GetX(), lhs.GetY() + rhs.GetY(), lhs.GetZ() + rhs.GetZ());
}


Note that this is a free function, not a member function of Vector. Prefer free functions, because they work in more cases (like multiplying an int to a Vector) and are thus more consistent.

This allows you to do this:

Vector v1{ 1, 2, 3 };
Vector v2{ 6, 5, 4 };
auto v3 = v1 + v2;


Now, if you do want to support unique_ptr you can do so basically the same way:

std::unique_ptr operator +(const std::unique_ptr &lhs,
    const std::unique_ptr &rhs){
    return std::make_unique(*lhs + *rhs); //uses above operator
}


This allows you to do this:

auto v1 = std::make_unique(1, 2, 3);
auto v2 = std::make_unique(4, 5, 7);
auto v3 = v1 + v2;


All in all you seem to have a pretty good grip on operator overloading and smart pointers already.

Code Snippets

Vector operator +(const Vector &lhs, const Vector &rhs){
    return Vector(lhs.GetX() + rhs.GetX(), lhs.GetY() + rhs.GetY(), lhs.GetZ() + rhs.GetZ());
}
Vector v1{ 1, 2, 3 };
Vector v2{ 6, 5, 4 };
auto v3 = v1 + v2;
std::unique_ptr<Vector> operator +(const std::unique_ptr<Vector> &lhs,
    const std::unique_ptr<Vector> &rhs){
    return std::make_unique<Vector>(*lhs + *rhs); //uses above operator
}
auto v1 = std::make_unique<Vector>(1, 2, 3);
auto v2 = std::make_unique<Vector>(4, 5, 7);
auto v3 = v1 + v2;

Context

StackExchange Code Review Q#78502, answer score: 5

Revisions (0)

No revisions yet.