How is my code?

**red floyd** · Jul 22 '05, 04:34 AM

Re: How is my code?

Darren Grant wrote:[color=blue]
> 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==(Poin t&, Point&);
> friend bool operator!=(Poin t&, 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+=(cons t 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::ostream& operator<<(std: :ostream&, const Point&);
>
> #endif
>
>
>
> #include <math.h>
>
> #include "Point.h"
>
> std::ostream& operator<<(std: :ostream& os, const Point& s) {
> os << "(" << s.X() << "," << s.Y() << ")";
> return os;
> }
>
> bool operator==(Poin t& p1, Point& p2) {
> return (p1._x == p2._x && p1._y == p2._y);
> }
>
> bool operator!=(Poin t& 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::operator +=(const Point& p) {
> _x += p._x;
> _y += p._y;
> return *this;
> }
>
> Point& Point::operator-=(const Point& p) {
> _x -= p._x;
> _y -= p._y;
> return *this;
> }
>
> double Point::dot(cons t Point& op) const {
> return _x * op.X() + _y * op.Y();
> }
>
> double Point::norm(voi d) const {
> return sqrt(_x * _x + _y * _y);
> }
>
> Point Point::normal(v oid) const {
> return *this / norm();
> }
>
> Point Point::proj(con st Point& u) const {
> return (*this * u.dot(*this)) / (norm() * norm());
> }
>
> Many thanks,
> Darren Grant[/color]

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

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

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

**Nicholas Hounsome** · Jul 22 '05, 04:34 AM

Re: How is my code?

"Darren Grant" <dg_6@hotmail.c om> wrote in message
news:opr0o65ghj 2gicz8@news.hot mail.com...[color=blue]
> 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
>[/color]

#include <iosfwd> instead 'cos you don't actually use ostream in the header
[color=blue]
> #include <iostream>
>
> class Point {[/color]

These don't need to be friends as the X() and Y() methods are inlined and
will do the job perfectly well
[color=blue]
> friend bool operator==(Poin t&, Point&);
> friend bool operator!=(Poin t&, Point&);[/color]

The difference between two points is not a point
[color=blue]
> friend Point operator-(const Point& l, const Point& r);[/color]

It is not meaningful to add two points
[color=blue]
> friend Point operator+(const Point& l, const Point& r);[/color]

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.
[color=blue]
> friend Point operator*(const Point& l, double r);[/color]

define behaviour for divide by 0
[color=blue]
> friend Point operator/(const Point& l, double r);
>
> public:
> Point() : _x(0), _y(0) { };
> Point(double x, double y) : _x(x), _y(y) { };
>[/color]

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.
[color=blue]
> void X(double x) { _x = x; } double X() const { return _x; }
> void Y(double y) { _y = y; } double Y() const { return _y; }
>
> Point& operator+=(cons t Point& p);
> Point& operator-=(const Point& p);
>
> double dot(const Point&) const;[/color]

(void) is only useful for C compatibility and since a class member cannot be
C compatible it has no place here.
[color=blue]
> double norm(void) const;
> Point normal(void) const;
> Point proj(const Point&) const;
>
> private:[/color]

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.
[color=blue]
> double _x, _y;
> };
>
> std::ostream& operator<<(std: :ostream&, const Point&);
>
> #endif
>
>
>
> #include <math.h>
>
> #include "Point.h"[/color]

#include <iostream>
[color=blue]
>
> std::ostream& operator<<(std: :ostream& os, const Point& s) {
> os << "(" << s.X() << "," << s.Y() << ")";
> return os;
> }
>
> bool operator==(Poin t& p1, Point& p2) {
> return (p1._x == p2._x && p1._y == p2._y);
> }
>
> bool operator!=(Poin t& p1, Point& p2) {
> return (p1._x != p2._x || p1._y != p2._y);
> }
>[/color]

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;
}
[color=blue]
> 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::operator +=(const Point& p) {
> _x += p._x;
> _y += p._y;
> return *this;
> }
>
> Point& Point::operator-=(const Point& p) {
> _x -= p._x;
> _y -= p._y;
> return *this;
> }
>
> double Point::dot(cons t Point& op) const {
> return _x * op.X() + _y * op.Y();
> }
>
> double Point::norm(voi d) const {
> return sqrt(_x * _x + _y * _y);
> }
>
> Point Point::normal(v oid) const {
> return *this / norm();
> }
>
> Point Point::proj(con st Point& u) const {
> return (*this * u.dot(*this)) / (norm() * norm());
> }
>
> Many thanks,
> Darren Grant[/color]

**Kamuela Franco** · Jul 22 '05, 04:35 AM

Re: How is my code?

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

Kamuela Franco

**Darren Grant** · Jul 22 '05, 04:35 AM

Re: How is my code?

On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
<nh007g8976@blu eyonder.co.uk> wrote:
[color=blue]
>
> "Darren Grant" <dg_6@hotmail.c om> wrote in message
> news:opr0o65ghj 2gicz8@news.hot mail.com...[color=green]
>> 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
>>[/color]
>
> #include <iosfwd> instead 'cos you don't actually use ostream in the
> header[/color]

Done.
[color=blue]
>[color=green]
>> #include <iostream>
>>
>> class Point {[/color]
>
> These don't need to be friends as the X() and Y() methods are inlined and
> will do the job perfectly well
>[color=green]
>> friend bool operator==(Poin t&, Point&);
>> friend bool operator!=(Poin t&, Point&);[/color][/color]

Ok, avoid friendship where you don't need, fair enough.
[color=blue]
> The difference between two points is not a point
>[color=green]
>> friend Point operator-(const Point& l, const Point& r);[/color]
>
> It is not meaningful to add two points
>[color=green]
>> friend Point operator+(const Point& l, const Point& r);[/color]
>
> Obviously this is intended to be scaling but even here you should[/color]

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:
[color=blue]
> implement
> *= and /= as members and implement * and /
> as non-friend free functions in terms of these.
>[color=green]
>> friend Point operator*(const Point& l, double r);[/color][/color]

Done.
[color=blue]
> define behaviour for divide by 0[/color]

if (r == 0)
throw std::domain_err or("divide by zero");
[color=blue][color=green]
>> friend Point operator/(const Point& l, double r);
>>
>> public:
>> Point() : _x(0), _y(0) { };
>> Point(double x, double y) : _x(x), _y(y) { };
>>[/color]
>
> 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.[/color]

I guess this is personal preference. I got this idea from Design Patterns
(http://www.amazon.com/exec/obidos/tg...2/102-9465371-
5694567?v=glanc e)
[color=blue]
> Use of uppercase for methods is inconsistent and contrary to most coding
> standards.
>[color=green]
>> void X(double x) { _x = x; } double X() const { return _x; }
>> void Y(double y) { _y = y; } double Y() const { return _y; }[/color][/color]

Ok, done.
[color=blue][color=green]
>> Point& operator+=(cons t Point& p);
>> Point& operator-=(const Point& p);
>>
>> double dot(const Point&) const;[/color]
>
> (void) is only useful for C compatibility and since a class member cannot
> be C compatible it has no place here.[/color]

Oh, ok.
[color=blue][color=green]
>> double norm(void) const;
>> Point normal(void) const;
>> Point proj(const Point&) const;
>>
>> private:[/color]
>
> 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.[/color]

Ok, changed.
[color=blue]
>[color=green]
>> double _x, _y;
>> };
>>
>> std::ostream& operator<<(std: :ostream&, const Point&);
>>
>> #endif
>>
>>
>>
>> #include <math.h>
>>
>> #include "Point.h"[/color]
>
> #include <iostream>
>[color=green]
>>
>> std::ostream& operator<<(std: :ostream& os, const Point& s) {
>> os << "(" << s.X() << "," << s.Y() << ")";
>> return os;
>> }
>>
>> bool operator==(Poin t& p1, Point& p2) {
>> return (p1._x == p2._x && p1._y == p2._y);
>> }
>>
>> bool operator!=(Poin t& p1, Point& p2) {
>> return (p1._x != p2._x || p1._y != p2._y);
>> }
>>[/color]
>
> 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;
> }[/color]

But then l is modified. Eg

m / 2; // m is changed
[color=blue][color=green]
>> 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::operator +=(const Point& p) {
>> _x += p._x;
>> _y += p._y;
>> return *this;
>> }
>>
>> Point& Point::operator-=(const Point& p) {
>> _x -= p._x;
>> _y -= p._y;
>> return *this;
>> }
>>
>> double Point::dot(cons t Point& op) const {
>> return _x * op.X() + _y * op.Y();
>> }
>>
>> double Point::norm(voi d) const {
>> return sqrt(_x * _x + _y * _y);
>> }
>>
>> Point Point::normal(v oid) const {
>> return *this / norm();
>> }
>>
>> Point Point::proj(con st Point& u) const {
>> return (*this * u.dot(*this)) / (norm() * norm());
>> }[/color][/color]

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

Regards,
Darren

**Nicholas Hounsome** · Jul 22 '05, 04:36 AM

Re: How is my code?

"Darren Grant" <dg_6@hotmail.c om> wrote in message
news:opr0q36ri1 2gicz8@news.hot mail.com...[color=blue]
> On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
> <nh007g8976@blu eyonder.co.uk> wrote:
>[/color]
[snip][color=blue]
> 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:
>[/color]

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]
[color=blue][color=green]
> >
> > 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[/color][/color]
methods[color=blue][color=green]
> > at all.[/color]
>
> I guess this is personal preference. I got this idea from Design Patterns
> (http://www.amazon.com/exec/obidos/tg...2/102-9465371-
> 5694567?v=glanc e)
>[/color]

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]
[color=blue][color=green]
> > 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;
> > }[/color]
>
> But then l is modified. Eg
>
> m / 2; // m is changed
>[/color]

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;

**Heiko Gottschling** · Jul 22 '05, 04:36 AM

Re: How is my code?

Darren Grant wrote:
[color=blue][color=green]
>> It is not meaningful to add two points[color=darkred]
>>> friend Point operator+(const Point& l, const Point& r);[/color]
>>[/color]
> 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>"[/color]

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

**Darren Grant** · Jul 22 '05, 04:36 AM

Re: How is my code?

On Fri, 26 Dec 2003 08:30:38 -0000, Nicholas Hounsome" nospace Hounsome
<@blueyonder.co .uk> wrote:
[color=blue]
>
> "Darren Grant" <dg_6@hotmail.c om> wrote in message
> news:opr0q36ri1 2gicz8@news.hot mail.com...[color=green]
>> On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
>> <nh007g8976@blu eyonder.co.uk> wrote:
>>[/color]
> [snip][color=green]
>> 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:
>>[/color]
>
> True but the class is called Point not Vector therefor I assume that is
> supposed to represent a point and not a vector hence....[/color]

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...
[color=blue]
> 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]
>[color=green][color=darkred]
>> >
>> > 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[/color][/color]
> methods[color=green][color=darkred]
>> > at all.[/color]
>>
>> I guess this is personal preference. I got this idea from Design
>> Patterns
>> (http://www.amazon.com/exec/obidos/tg...2/102-9465371-
>> 5694567?v=glanc e)
>>[/color]
>
> Which pattern?[/color]

Right at the end, in 'Foundation classes'. Only the class definition
is given, not the implementation.
[color=blue]
> 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]
>[color=green][color=darkred]
>> > 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;
>> > }[/color]
>>
>> But then l is modified. Eg
>>
>> m / 2; // m is changed
>>[/color]
>
> 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;[/color]

Damn. I left the & in. :)

Cheers,
Darren

**Derek** · Jul 22 '05, 04:36 AM

Re: How is my code?

"Darren Grant" wrote:[color=blue][color=green]
> > True but the class is called Point not Vector therefor I
> > assume that is supposed to represent a point and not a
> > vector hence....[/color]
>
> 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...[/color]

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.

How is my code?

How is my code?

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment