How is my code?

D

Darren Grant

Hi there,

I'm learning C++, and just wrote a 2D point class. Would some kind soul
be able to look over my code and give me any constructive criticism they
can think of?

#ifndef POINT_H
#define POINT_H

#include <iostream>

class Point {
friend bool operator==(Point&, Point&);
friend bool operator!=(Point&, Point&);
friend Point operator-(const Point& l, const Point& r);
friend Point operator+(const Point& l, const Point& r);
friend Point operator*(const Point& l, double r);
friend Point operator/(const Point& l, double r);

public:
Point() : _x(0), _y(0) { };
Point(double x, double y) : _x(x), _y(y) { };

void X(double x) { _x = x; } double X() const { return _x; }
void Y(double y) { _y = y; } double Y() const { return _y; }

Point& operator+=(const Point& p);
Point& operator-=(const Point& p);

double dot(const Point&) const;
double norm(void) const;
Point normal(void) const;
Point proj(const Point&) const;

private:
double _x, _y;
};

std::eek:stream& operator<<(std::eek:stream&, const Point&);

#endif



#include <math.h>

#include "Point.h"

std::eek:stream& operator<<(std::eek:stream& os, const Point& s) {
os << "(" << s.X() << "," << s.Y() << ")";
return os;
}

bool operator==(Point& p1, Point& p2) {
return (p1._x == p2._x && p1._y == p2._y);
}

bool operator!=(Point& p1, Point& p2) {
return (p1._x != p2._x || p1._y != p2._y);
}

Point operator+(const Point& l, const Point& r) {
Point p = l;
p += r;
return p;
}

Point operator-(const Point& l, const Point& r) {
Point p = l;
p -= r;
return p;
}

Point operator*(const Point &l, double r) {
Point p = l;
p._x *= r;
p._y *= r;
return p;
}

Point operator/(const Point &l, double r) {
Point p = l;
p._x /= r;
p._y /= r;
return p;
}

Point& Point::eek:perator+=(const Point& p) {
_x += p._x;
_y += p._y;
return *this;
}

Point& Point::eek:perator-=(const Point& p) {
_x -= p._x;
_y -= p._y;
return *this;
}

double Point::dot(const Point& op) const {
return _x * op.X() + _y * op.Y();
}

double Point::norm(void) const {
return sqrt(_x * _x + _y * _y);
}

Point Point::normal(void) const {
return *this / norm();
}

Point Point::proj(const Point& u) const {
return (*this * u.dot(*this)) / (norm() * norm());
}

Many thanks,
Darren Grant
 
R

red floyd

Darren said:
Hi there,

I'm learning C++, and just wrote a 2D point class. Would some kind soul
be able to look over my code and give me any constructive criticism they
can think of?

#ifndef POINT_H
#define POINT_H

#include <iostream>

class Point {
friend bool operator==(Point&, Point&);
friend bool operator!=(Point&, Point&);
friend Point operator-(const Point& l, const Point& r);
friend Point operator+(const Point& l, const Point& r);
friend Point operator*(const Point& l, double r);
friend Point operator/(const Point& l, double r);

public:
Point() : _x(0), _y(0) { };
Point(double x, double y) : _x(x), _y(y) { };

void X(double x) { _x = x; } double X() const { return _x; }
void Y(double y) { _y = y; } double Y() const { return _y; }

Point& operator+=(const Point& p);
Point& operator-=(const Point& p);

double dot(const Point&) const;
double norm(void) const;
Point normal(void) const;
Point proj(const Point&) const;

private:
double _x, _y;
};

std::eek:stream& operator<<(std::eek:stream&, const Point&);

#endif



#include <math.h>

#include "Point.h"

std::eek:stream& operator<<(std::eek:stream& os, const Point& s) {
os << "(" << s.X() << "," << s.Y() << ")";
return os;
}

bool operator==(Point& p1, Point& p2) {
return (p1._x == p2._x && p1._y == p2._y);
}

bool operator!=(Point& p1, Point& p2) {
return (p1._x != p2._x || p1._y != p2._y);
}

Point operator+(const Point& l, const Point& r) {
Point p = l;
p += r;
return p;
}

Point operator-(const Point& l, const Point& r) {
Point p = l;
p -= r;
return p;
}

Point operator*(const Point &l, double r) {
Point p = l;
p._x *= r;
p._y *= r;
return p;
}

Point operator/(const Point &l, double r) {
Point p = l;
p._x /= r;
p._y /= r;
return p;
}

Point& Point::eek:perator+=(const Point& p) {
_x += p._x;
_y += p._y;
return *this;
}

Point& Point::eek:perator-=(const Point& p) {
_x -= p._x;
_y -= p._y;
return *this;
}

double Point::dot(const Point& op) const {
return _x * op.X() + _y * op.Y();
}

double Point::norm(void) const {
return sqrt(_x * _x + _y * _y);
}

Point Point::normal(void) const {
return *this / norm();
}

Point Point::proj(const Point& u) const {
return (*this * u.dot(*this)) / (norm() * norm());
}

Many thanks,
Darren Grant
Click to expand...

since you overloaded * and / for scalars, for semantic completeness, you should probably overload *= and /= as well.

Point& Point::eek:perator*=(double d)
{
_x *= d;
_y *= d;
return *this;
}

Point& Point::eek:perator/=(double d)
{
_x /= d;
_y /= d;
return *this;
}
 
N

Nicholas Hounsome

Darren Grant said:
Hi there,

I'm learning C++, and just wrote a 2D point class. Would some kind soul
be able to look over my code and give me any constructive criticism they
can think of?

#ifndef POINT_H
#define POINT_H
Click to expand...

#include said:
#include <iostream>

class Point {
Click to expand...

These don't need to be friends as the X() and Y() methods are inlined and
will do the job perfectly well
friend bool operator==(Point&, Point&);
friend bool operator!=(Point&, Point&);
Click to expand...

The difference between two points is not a point
friend Point operator-(const Point& l, const Point& r);
Click to expand...

It is not meaningful to add two points
friend Point operator+(const Point& l, const Point& r);
Click to expand...

Obviously this is intended to be scaling but even here you should implement
*= and /= as members and implement * and /
as non-friend free functions in terms of these.
friend Point operator*(const Point& l, double r);
Click to expand...

define behaviour for divide by 0
friend Point operator/(const Point& l, double r);

public:
Point() : _x(0), _y(0) { };
Point(double x, double y) : _x(x), _y(y) { };
Click to expand...

I would prefer not to have manipulators for X and Y - It is cleaner to just
use constructors as in
X a;
a = Point(a.X(),42);
For small classes with simple ctors I usually prefer no mutating methods at
all.

Use of uppercase for methods is inconsistent and contrary to most coding
standards.
void X(double x) { _x = x; } double X() const { return _x; }
void Y(double y) { _y = y; } double Y() const { return _y; }

Point& operator+=(const Point& p);
Point& operator-=(const Point& p);

double dot(const Point&) const;
Click to expand...

(void) is only useful for C compatibility and since a class member cannot be
C compatible it has no place here.
double norm(void) const;
Point normal(void) const;
Point proj(const Point&) const;

private:
Click to expand...

I'm not sure on the exact details of when you can or cannot use leading _
and nor are most other C++ programmers.
us x_,y_ or m_x,m_y and you'll be certain that it wont clash with
implementation stuff.
double _x, _y;
};

std::eek:stream& operator<<(std::eek:stream&, const Point&);

#endif



#include <math.h>

#include "Point.h"
Click to expand...

#include said:
std::eek:stream& operator<<(std::eek:stream& os, const Point& s) {
os << "(" << s.X() << "," << s.Y() << ")";
return os;
}

bool operator==(Point& p1, Point& p2) {
return (p1._x == p2._x && p1._y == p2._y);
}

bool operator!=(Point& p1, Point& p2) {
return (p1._x != p2._x || p1._y != p2._y);
}
Click to expand...

The siimplest and potentially most efficient way to do the following sort
of thing is -

Point operator+(Point l,const Point&r)
{
return l+=r;
}
 
K

Kamuela Franco

Only thing I would say is use '#include <cmath>' to be more standards
compliant.

Kamuela Franco
 
D

Darren Grant

#include <iosfwd> instead 'cos you don't actually use ostream in the
header
Done.


These don't need to be friends as the X() and Y() methods are inlined and
will do the job perfectly well
Click to expand...

Ok, avoid friendship where you don't need, fair enough.
The difference between two points is not a point


It is not meaningful to add two points


Obviously this is intended to be scaling but even here you should
Click to expand...

Oh? "To add two (or more) vectors together, all you have to do is add the
respective components together." "u + v = <u_x + v_x, u_y + v_y>"
(from Tricks of the 3D Game Programming Gurus by Andre Lamothe).

Similarly for subtraction.

Scaling is done with multiplication by a scalar:
implement
*= and /= as members and implement * and /
as non-friend free functions in terms of these.

Done.

define behaviour for divide by 0
Click to expand...

if (r == 0)
throw std::domain_error("divide by zero");
I would prefer not to have manipulators for X and Y - It is cleaner to
just
use constructors as in
X a;
a = Point(a.X(),42);
For small classes with simple ctors I usually prefer no mutating methods
at all.
Click to expand...

I guess this is personal preference. I got this idea from Design Patterns
(http://www.amazon.com/exec/obidos/tg/detail/-/0201633612/102-9465371-
5694567?v=glance)
Use of uppercase for methods is inconsistent and contrary to most coding
standards.
Click to expand...

Ok, done.
(void) is only useful for C compatibility and since a class member cannot
be C compatible it has no place here.
Click to expand...

Oh, ok.
I'm not sure on the exact details of when you can or cannot use leading _
and nor are most other C++ programmers.
us x_,y_ or m_x,m_y and you'll be certain that it wont clash with
implementation stuff.
Click to expand...

Ok, changed.
The siimplest and potentially most efficient way to do the following
sort
of thing is -

Point operator+(Point l,const Point&r)
{
return l+=r;
}
Click to expand...

But then l is modified. Eg

m / 2; // m is changed

Cool, thanks very much for your help. I definately learned a lot.

Regards,
Darren
 
N

Nicholas Hounsome

Darren Grant said:
On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
[snip]
Oh? "To add two (or more) vectors together, all you have to do is add the
respective components together." "u + v = <u_x + v_x, u_y + v_y>"
(from Tricks of the 3D Game Programming Gurus by Andre Lamothe).

Similarly for subtraction.

Scaling is done with multiplication by a scalar:
Click to expand...

True but the class is called Point not Vector therefor I assume that is
supposed to represent a point and not a vector hence....

The naming of things is probably the most important and underrated part of
OO design -
consider bery carefully what a class is and have it do only appropriate
methods for that abstraction and users/maintainers will be able to get
around your code using their natural intuition and doamin knowledge.

[snip]
I guess this is personal preference. I got this idea from Design Patterns
(http://www.amazon.com/exec/obidos/tg/detail/-/0201633612/102-9465371-
5694567?v=glance)
Click to expand...

Which pattern?
You may find that immutable types are desirable in multithreaded
applications also - following on from the previous comment - the abstraction
is a point not a pair of x,y coordinates - it's not an absolute - more a
question of emphasis - you may have read several books going on about how
you might want to use polar coordinates as the internal representation (No
I've never found a use for it either).
[snip]
But then l is modified. Eg

m / 2; // m is changed
Click to expand...

No it isn't because l is not passed by reference so it is a copy of m i.e.
the compiler has automatically done what you did by writing
Point temp = l;
 
H

Heiko Gottschling

Darren said:
Oh? "To add two (or more) vectors together, all you have to do is add the
respective components together." "u + v = <u_x + v_x, u_y + v_y>"
Click to expand...

Yes, but points are not vectors. If you want a vector, call the class
'Vector'! Adding two points does not make sense, however adding a vector to
a point does! Having two classes, 'Point' (P) and 'Vector' (V), the
following +/- operations should be supported:

V + V = V
V - V = V
P + V = P
P - P = V

Everything else (like 'P + P') is undefined!

cu
Heiko
 
D

Darren Grant

Darren Grant said:
On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
[snip]
Oh? "To add two (or more) vectors together, all you have to do is add
the
respective components together." "u + v = <u_x + v_x, u_y + v_y>"
(from Tricks of the 3D Game Programming Gurus by Andre Lamothe).

Similarly for subtraction.

Scaling is done with multiplication by a scalar:
Click to expand...

True but the class is called Point not Vector therefor I assume that is
supposed to represent a point and not a vector hence....
Click to expand...

True, I didn't call it Vector because of the STL vector, and also it's
not as general as an n-dimensional vector - it's only a 2D vector.

But... what's the difference between a 2D point and a 2D vector? My reason
for calling the class Point is because I was under the impression there is
none...
The naming of things is probably the most important and underrated part
of OO design -
consider bery carefully what a class is and have it do only appropriate
methods for that abstraction and users/maintainers will be able to get
around your code using their natural intuition and doamin knowledge.

[snip]
I guess this is personal preference. I got this idea from Design
Patterns
(http://www.amazon.com/exec/obidos/tg/detail/-/0201633612/102-9465371-
5694567?v=glance)
Click to expand...

Which pattern?
Click to expand...

Right at the end, in 'Foundation classes'. Only the class definition
is given, not the implementation.
You may find that immutable types are desirable in multithreaded
applications also - following on from the previous comment - the
abstraction
is a point not a pair of x,y coordinates - it's not an absolute - more a
question of emphasis - you may have read several books going on about how
you might want to use polar coordinates as the internal representation
(No
I've never found a use for it either).
[snip]
But then l is modified. Eg

m / 2; // m is changed
Click to expand...

No it isn't because l is not passed by reference so it is a copy of m
i.e.
the compiler has automatically done what you did by writing
Point temp = l;
Click to expand...

Damn. I left the & in. :)

Cheers,
Darren
 
D

Derek

Darren Grant said:
True, I didn't call it Vector because of the STL vector, and
also it's not as general as an n-dimensional vector - it's
only a 2D vector.

But... what's the difference between a 2D point and a 2D
vector? My reason for calling the class Point is because I was
under the impression there is none...
Click to expand...

Most graphics textbooks have introductory chapters that describe the
differences between points and vectors, which are not the same as far
as mathematical operations are concerned. For example, P+V=P, P-P=V,
P+P is not strictly legal, etc. If you want to go further, there is a
third class, normals, which are vectors that are treated differently
in certain respects. (Incidentally, the existence of normals makes
your normal() member function a potential source of confusion; perhaps
normalize() would be a better name.)

As Nicholas pointed out, naming is very important. You could call
your class Vec2d or Vector2d to more precisely describe its meaning,
and to eliminate any confusion with std::vector. Or put it in a
namespace, like Math2d, so Math2d::Vector will be its fully-qualified
name.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

beginner's question concerning operator overloading. 5
How Methods Return Objects 25
Lexical Analysis on C++ 1
How to insert immutable object into the stl map 2
'access private member' error 5
Drawing missing in bitmap in a pure C win32 program 4
R-value references and performance 2
is this portable 32

Members online

Total: 31 (members: 1, guests: 30)
Robots: 373

Forum statistics

Threads
473,769
Messages
2,569,582
Members
45,070
Latest member
BiogenixGummies

Latest Threads

Top