To Veterans: Have a better way of coding this?

**Alphacentric666** · 12-28-2002

Welcome!!

I'm adding my source code in here to recieve criticism from other programmers.

**matheo917** · 12-28-2002

just for reference...what's this supposed to be???

**Stoned_Coder** · 12-28-2002

Im sorry to say this but its not too good at all.

first u mix include headers using some using std namespace and some not.This isnt a good start especially as none of those headers are used in main.h.Then using namespace std brings the whole of the namespace into scope when in reality you are not using any of it.
You should prefer using initialisation list instead of constructor body to initialise members.Only use body if u have to.No checking either for bounds. Not sure how class is to be used. Will negative values here make sense? Should u check X and Y before initialising members.
Just for aesthitic value a virtual destructor wouldnt have gone amiss. That would make this class more value as a base class.
operator + is ok as u have done it but again would this make sense still if x and y are negative?
friend int main() WTF what,why,are you trying to do? Why on earth should main be a friend of this class.Thats preposterous.
Information need not be a friend either. It uses accessor functions to access the classes internal data and does not need direct access.
Magnitude needs rewriting and also does not need to be a friend for same reasons as Information.
Your accessor functions are laughable. Where is your const correctness?!
Another word of advice.... In c++ we have references. They can dramatically reduce the need for passing pointers into functions.

I'd seriously consider a rewrite.

**matheo917** · 12-28-2002

i strongly agree w/ Stonge_Coder...

**Nick** · 12-28-2002

Here is a better way. You have to be careful
with the normalization since you can have 0 vectors.
Right now I comparing floating point numbers to 0.0 but to
make it more robust you should compare it to a range of
numbers near 0. You can improve this further by maybe using
templates, adding substracting, multiplication etc. Another
way to handle the normalization is to return another vector
and make the action function const.

Code:

#ifndef VECTOR_H__
#define VECTOR_H__

#include <stdexcept>
#include <cmath>

class Vector {
    double x;
    double y;
public:
    Vector(double new_x = 0.0, double new_y = 0.0) : x(new_x), y(new_y) { }
    Vector& operator+=(const Vector& rhs) {
        x += rhs.x;
        y += rhs.y;

        return *this;
    }

    Vector operator+(const Vector& rhs) {
        Vector& v = *this;
        v += rhs;
        return v;
    }

    double get_x() const { return x; }
    double get_y() const { return y; }
    
    double magnitude() const {
        return std::sqrt(x * x + y * y);
    }

    void normalize() {
        double m = magnitude();
        if (m == 0.0)
            throw std::runtime_error("Divide by 0");
        
        x /= m;
        y /= m;
    }
};

#endif

#include <iostream>
using std::cout;
using std::endl;

#include "Vector.h"


int main(void)
{
    Vector v(1, 9);
    Vector u(4, 5);
    Vector w;
    
    cout << "v = (" << v.get_x() << ", " << v.get_y() << ")" << endl;
    cout << "u = (" << u.get_x() << ", " << u.get_y() << ")" << endl;
    w = u + v;
    cout << "w = u + v" << endl;
    cout << "w = (" << w.get_x() << ", " << w.get_y() << ")" << endl;
    
    cout << "v's magnitude = " << v.magnitude() << endl;
    cout << "u's magnitude = " << u.magnitude() << endl;
    cout << "normalizing" << endl;
    v.normalize();
    u.normalize();
    cout << "v = (" << v.get_x() << ", " << v.get_y() << ")" << endl;
    cout << "u = (" << u.get_x() << ", " << u.get_y() << ")" << endl;

    cout << "v's magnitude = " << v.magnitude() << endl;
    cout << "u's magnitude = " << u.magnitude() << endl;
       
    return 0;
}

**joshdick** · 12-29-2002

Originally posted by Stoned_Coder
Just for aesthitic value a virtual destructor wouldnt have gone amiss. That would make this class more value as a base class.

How would that be done? I thought that delete could only be used if new was used. What would you put into the implementation of the destructor, then?

**Stoned_Coder** · 12-29-2002

declare it in the class as virtual ~classname(); and in the implementation file add a do nothing implementation.

class MyClass
{
virtual ~MyClass();
};

MyClass::~MyClass() {}

If you have no intention of ever using this as a base class then this is probably overkill,if on the other hand you intend to derive from this class and use polymorphically then a virtual destructor is necessary.

**Alphabird32** · 12-29-2002

Heres the revision

**Stoned_Coder** · 12-29-2002

ok here we go again.

accessor functions should be const.
PrimTypebyValue GetPrivateMember()const { return PrivateMember;}
or
const UserDefTypebyConstRef& GetPrivateMember()const { return PrivateMember;}

operator += should return *this

iostream is not used in main.h it should be included in main.cpp instead

main does never need to be prototyped.

Information() should take a reference not a pointer.

(*Vector).getX() is better written Vector->getX()

**Stoned_Coder** · 12-29-2002

Nick your operator + is wrong. It changes *this....

a=b+c you dont expect b to be changed. *this should be const throughout operator +

**Alphacentric666** · 12-29-2002

Why should accessor functions be const?

**Stoned_Coder** · 12-29-2002

const objects are only allowed to call upon const member functions and cannot call non const member funcs.

i.e.

const CVector vec(10,10); // a const object
int a = vec.getX(); // ERROR unless function is const member and it should be because it doesnt change the state of the object.

Thread: To Veterans: Have a better way of coding this?

Thread Tools

Search Thread

Display

To Veterans: Have a better way of coding this?

Similar Threads

The chicken and the egg problem... trying to learn coding but having trouble starting

Coding Guideline ....!!

Before Coding

Coding Contest....