Do I have to instantiate this class? Or is this a bad separation of responsibilities?

Question

I'm practicing with classes and I created a program that creates a vector and calculates the magnitude of it. I separated the code so that one class is a vector class and a different class handles the math. My question is, is this incorrect? I have to instantiate my math class in order to be able to use it, and it looks clunky, even though it works.

Main:

int main()
{
    // Instantiate math library... ?
    Math math; // <-- Instantiating math library here
    Vector3D *test = new Vector3D("Test vector 1", 0, 0, 1);
    printVector(test);

    // Calling math library to calculate magnitude
    std::cout << "Test vector 1 magnitude: " << math.magnitude(test) << std::endl;


    return 0;
}

Vector Class:

class Vector3D {
    private:
        std::string     name;
        double          x;
        double          y;
        double          z;

    public:
        Vector3D();
        Vector3D(std::string, double, double, double);

        // Setters
        void setName(std::string);
        void setX(double);
        void setY(double);
        void setZ(double);

        // Getters
        std::string getName() const;
        double getX() const;
        double getY() const;
        double getZ() const;

        // Operators
        Vector3D& operator *=(double s); // Scalar multiplication
        Vector3D& operator /=(double s); // Scalar division


};

Math Class:

class Math {
    public:
        double magnitude(const Vector3D* v);
};

It's not wrong, but one of the ideas behind object-oriented programming is to package code together with the data that it will operate on. Given that, I think it would be more useful to make magnitude() a member function of the Vector3D class than to keep it separate. — Jeremy Friesner, Jan 08 '17 at 06:11
Too broad. Maybe http://codereview.stackexchange.com/ would be a better fit for this question. — Christian Hackl, Jan 08 '17 at 12:44

score 8 · Answer 1 · answered Jan 08 '17 at 05:50

8

Classes serve to encapsulate state. Math has no state, therefore it should be a namespace, not a class.

answered Jan 08 '17 at 05:50

Don Reba

13,814
3
48
61

score 6 · Answer 2 · answered Jan 08 '17 at 05:51

My question is, is this incorrect?

According to Scott Meyers, non-member functions improve encapsulation.

If you can use a non-member function to implement a function, it is better to do that.

Using the member function of another class to do the computation does not make sense. It will be better to use a namespace and define the function in the namespace.

score 2 · Accepted Answer · edited May 23 '17 at 11:54

The extra class is certainly not unusually complex for C++ (see, for example this perfectly normative code for generating a random number: https://stackoverflow.com/a/13445752/5478284.)

Because the math class contains no relevant information about the vectors, only functions, you should not have to instantiate it: you could use static functions. At this point, though, as other answers mention, you basically have a namespace, which I would recommend designing to contain both useful vector functions and the Vector3D class.

Also, I would rename it as it currently stands. math is an incredibly broad name. (Perhaps that's why it feels so bad to instantiate it before small operations.)

score 1 · Answer 4 · answered Jan 13 '17 at 16:40

The first inexcusable bundling of responsibilities you have is including a string in your class. Mathematical values shouldn't have names. The second is the existence of setters. The third is the the 'get' in your getters.

Mathematical value types should be modeled after 'int', not 'object'.

class Vector3D
{

private: // fields

    double _x;
    double _y;
    double _z;

public: // constructors

    constexpr explicit Vector3D ( double const x, double const y, double const z )
        : _x {x}, _y {y}, _z {z} { }

public: // accessors

    constexpr double x () const { return _x; }
    constexpr double y () const { return _y; }
    constexpr double z () const { return _z; }

};

constexpr Vector3D operator * ( Vector3D const left, double const right )
{
    return Vector3D { left.x () * right, left.y () * right, left.z () * right };
}

constexpr Vector3D operator / ( Vector3D const left, double const right )
{
    return Vector3D { left.x () / right, left.y () / right, left.z () / right };
}


Vector3D & operator *= ( Vector3D & left, double const right )
{
    return left = left * right;
}

Vector3D & operator /= ( Vector3D & left, double const right )
{
    return left = left / right;
}

When you instantiate a vector3d, you should drop the new and the pointer. Just construct it on the stack:

auto const point = Vector3D ( 0, 0, 1 );

Also, don't copy Java.

Do I have to instantiate this class? Or is this a bad separation of responsibilities?

4 Answers4