473,322 Members | 1,408 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,322 software developers and data experts.

How is my code?

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::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==(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::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(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
Jul 22 '05 #1
8 1200
Darren Grant wrote:
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::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==(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::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(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


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;
}
Jul 22 '05 #2

"Darren Grant" <dg**@hotmail.com> wrote in message
news:op**************@news.hotmail.com...
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 <iosfwd> instead 'cos you don't actually use ostream in the header
#include <iostream>

class Point {
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&);
The difference between two points is not a point
friend Point operator-(const Point& l, const Point& r);
It is not meaningful to add two points
friend Point operator+(const Point& l, const Point& r);
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);
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) { };

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;
(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:
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::ostream& operator<<(std::ostream&, const Point&);

#endif

#include <math.h>

#include "Point.h"
#include <iostream>

std::ostream& operator<<(std::ostream& 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);
}

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;
}
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(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

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

Kamuela Franco
Jul 22 '05 #4
On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
<nh********@blueyonder.co.uk> wrote:

"Darren Grant" <dg**@hotmail.com> wrote in message
news:op**************@news.hotmail.com...
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 <iosfwd> instead 'cos you don't actually use ostream in the
header


Done.
#include <iostream>

class Point {
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&);
Ok, avoid friendship where you don't need, fair enough.
The difference between two points is not a point
friend Point operator-(const Point& l, const Point& r);
It is not meaningful to add two points
friend Point operator+(const Point& l, const Point& r);


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


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.
friend Point operator*(const Point& l, double r);

Done.
define behaviour for divide by 0
if (r == 0)
throw std::domain_error("divide by zero");
friend Point operator/(const Point& l, double r);

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


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.


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=glance)
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; }
Ok, done.
Point& operator+=(const Point& p);
Point& operator-=(const Point& p);

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


Oh, ok.
double norm(void) const;
Point normal(void) const;
Point proj(const Point&) const;

private:


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.


Ok, changed.
double _x, _y;
};

std::ostream& operator<<(std::ostream&, const Point&);

#endif

#include <math.h>

#include "Point.h"


#include <iostream>

std::ostream& operator<<(std::ostream& 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);
}


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;
}


But then l is modified. Eg

m / 2; // m is changed
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(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());
}


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

Regards,
Darren
Jul 22 '05 #5

"Darren Grant" <dg**@hotmail.com> wrote in message
news:op**************@news.hotmail.com...
On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
<nh********@blueyonder.co.uk> wrote:
[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:


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 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.


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=glance)


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]
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;
}


But then l is modified. Eg

m / 2; // m is changed


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;
Jul 22 '05 #6
Darren Grant wrote:
It is not meaningful to add two points
friend Point operator+(const Point& l, const Point& r);

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>"


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

Jul 22 '05 #7
On Fri, 26 Dec 2003 08:30:38 -0000, Nicholas Hounsome" nospace Hounsome
<@blueyonder.co.uk> wrote:

"Darren Grant" <dg**@hotmail.com> wrote in message
news:op**************@news.hotmail.com...
On Wed, 24 Dec 2003 21:42:27 -0000, Nicholas Hounsome
<nh********@blueyonder.co.uk> wrote:
[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:


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


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 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.
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=glance)


Which pattern?


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]
> 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;
> }


But then l is modified. Eg

m / 2; // m is changed


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;


Damn. I left the & in. :)

Cheers,
Darren
Jul 22 '05 #8
"Darren Grant" wrote:
True but the class is called Point not Vector therefor I
assume that is supposed to represent a point and not a
vector hence....


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...


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.
Jul 22 '05 #9

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

51
by: Mudge | last post by:
Please, someone, tell me why OO in PHP is better than procedural.
9
by: bigoxygen | last post by:
Hi. I'm using a 3 tier FrontController Design for my web application right now. The problem is that I'm finding to have to duplicate a lot of code for similar functions; for example, listing...
4
by: jason | last post by:
Hello. Newbie on SQL and suffering through this. I have two tables created as such: drop table table1; go drop table table2; go
16
by: Dario de Judicibus | last post by:
I'm getting crazy. Look at this code: #include <string.h> #include <stdio.h> #include <iostream.h> using namespace std ; char ini_code = {0xFF, 0xFE} ; char line_sep = {0x20, 0x28} ;
109
by: Andrew Thompson | last post by:
It seems most people get there JS off web sites, which is entirely logical. But it is also a great pity since most of that code is of such poor quality. I was looking through the JS FAQ for any...
5
by: ED | last post by:
I currently have vba code that ranks employees based on their average job time ordered by their region, zone, and job code. I currently have vba code that will cycle through a query and ranks each...
0
by: Namratha Shah \(Nasha\) | last post by:
Hey Guys, Today we are going to look at Code Access Security. Code access security is a feature of .NET that manages code depending on its trust level. If the CLS trusts the code enough to...
18
by: Joe Fallon | last post by:
I have some complex logic which is fairly simply to build up into a string. I needed a way to Eval this string and return a Boolean result. This code works fine to achieve that goal. My...
37
by: Alan Silver | last post by:
Hello, Newbie here, so please forgive what is probably a basic question ... I see a lot of discussion about "code behind", which if I have understood correctly, means that the script code goes...
171
by: tshad | last post by:
I am just trying to decide whether to split my code and uses code behind. I did it with one of my pages and found it was quite a bit of trouble. I know that most people (and books and articles)...
0
by: DolphinDB | last post by:
Tired of spending countless mintues downsampling your data? Look no further! In this article, you’ll learn how to efficiently downsample 6.48 billion high-frequency records to 61 million...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
1
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
0
by: ArrayDB | last post by:
The error message I've encountered is; ERROR:root:Error generating model response: exception: access violation writing 0x0000000000005140, which seems to be indicative of an access violation...
1
by: CloudSolutions | last post by:
Introduction: For many beginners and individual users, requiring a credit card and email registration may pose a barrier when starting to use cloud servers. However, some cloud server providers now...
1
by: Defcon1945 | last post by:
I'm trying to learn Python using Pycharm but import shutil doesn't work
1
by: Shællîpôpï 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
by: af34tf | last post by:
Hi Guys, I have a domain whose name is BytesLimited.com, and I want to sell it. Does anyone know about platforms that allow me to list my domain in auction for free. Thank you

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.