tags:

views:

1486

answers:

11

I'm learning C++ (and programming in general) and I'm trying to make both a Point class and a Line class.

A line should be composed of 2 point objects.

Can the C++ gurus look over my work and tell me if this is how you should appropriately use pointers, references and classes?

class Point
{
    private:
        int x, y;
    public:
        Point() : x(0), y(0) {}
        Point(int x, int y) : x(x), y(y) {}
}

class Line
{
    private:
        Point *p1;
        Point *p2;
    public:
        Line(Point &p1, Point &p2) : p1(p1), p2(p2) {}

        void setPoints(Point &p1, Point &p2)
        {
            this->p1 = p1;
            this->p2 = p2;
        }
}
+4  A: 

You should not be using pointers at all in your code. Use actual objects. Pointers are actually used quite rarely in C++.

class Point
{
    private:
        int x, y;
    public:
        Point() : x(0), y(0) {}
        Point(int x, int y) : x(x), y(y) {}
}

class Line
{
    private:
        Point p1;
        Point p2;
    public:
        Line(const Point & p1, const Point & p2 ) : p1(p1), p2(p2) {}

        void setPoints( const Point & ap1, const Point & ap2)
        {
            p1 = ap1;
            p2 = ap2;
        }
}
anon
Pointers are not used rarely in C++. You obviously haven't done any GUI programming, where trees are involved. =]
strager
doh, I guess in over 20 years of C++ programming I never came across a tree - silly me.
anon
I don't need to use pointers to build a tree.
Dave Van den Eynde
@zabzonk, thank you for this.
Can you explain your use of const in the Line constructor and setPoints method?
@zabzonk: Ran into a graph, then? As a matter of fact, the need for pointers rises from the poblem domain.
xtofl
@ThePony see http://www.parashift.com/c++-faq-lite/const-correctness.html for a description of `const`
ChrisW
@ChrisW, Thanks a million.
I don't get the downvoting for this answer. @zabzonk, maybe if you edited the answer to say "Raw pointers should be used quite rarely in modern C++" it would be better? I really think you have a point, but it seems to be misunderstood. There really is no need for raw pointers in this code.
mghie
It is certain that c++ allows you to greatly reduce the number of pointers you must use (NB: I suck at this, c reflexes), but "rarely" might be too strong a word here.
dmckee
+1  A: 

I would prefer this...

class Point
{
    private:
        int x, y;
    public:
        Point() : x(0), y(0) {}
        Point(int x, int y) : x(x), y(y) {}
}

class Line
{
    private:
        Point p1;
        Point p2;
    public:
        Line(const Point &p1, const Point &p2) : p1(p1), p2(p2) {}

        void setPoints(const Point &p1, const Point &p2)
        {
            this->p1 = p1;
            this->p2 = p2;
        }
}
SDX2000
p1 = p1; Surely some mistake here ?
anon
Yup you are right. Now fixed. :)
SDX2000
+2  A: 

There is no need to use pointers in your Line class.

Also, the following line is incorrect:

Line(Point &p1, Point &p2) : p1(p1), p2(p2) {}

Why? You are assigning a Point & (p1) to a Point * (Line::p1), which is illegal. You'd want pointers there.

Your Point class has no way to modify its data. Not too useful...

A Line class for me would look something like this:

class Line
{
    private:
        Point p1, p2;

    public:
        Line()
        {
        }

        Line(Point start, Point end) :
            p1(start), p2(end)
        {
        }

        const Point &startPoint() const
        {
            return p1;
        }

        Point &startPoint()
        {
            return p1;
        }

        const Point &endPoint() const
        {
            return p2;
        }

        Point &endPoint()
        {
            return p2;
        }
};

You can now edit your line like this:

Line line;
line.startPoint() = Point(4, 2);
line.endPoint() = Point(6, 9);
strager
you are not a believer in the use of const then?
anon
sorry, ignore the last - misread the code
anon
Why do you have 2 get methods for each point, one with const and one without?
The non-const getter allows the caller to assign to the member variable.
Dave Van den Eynde
Well, why have a const one then in the first place I guess? What is the idea behind that.
@ThePony, In case you have e.g. a const Line. Google "const correctness" for C++. It's a topic which is often misunderstood by new C++ programmers.
strager
+1  A: 

A couple of things I noticed :

  • You can combine both of your point constructors into a single constructor with default values.
  • Your usage of pointers is quite unnecessary. Use the object itself.
  • You're using both pointers and references interchangeably. Don't mix them up, or see the last point.
sykora
A single constructor with default values would also allow Point(1) to construct a point (with the same effect as Point(1,0)). I prefer the two constructors.
Renze de Waal
+1  A: 

I see little value in making Point's pointers (other than for irony value). Your Point takes 8 bytes on a 32 bit system (2 int's). A pointer takes 4 bytes. you're saving 4 bytes.

As for correctness, your Line constructor takes references, but you're assigning them to pointers. That shouldn't even compile. You're also doing the same thing in setPoints. It would be better to simply make the two points actual objects and copying their values.

Mystere Man
Huh? How does the pointer save 4 bytes - it will point to the 8 bytes of a Point instance, meaning it *adds* 4 bytes? This is IMHO a little misleading.
mghie
I was referring to the class itself. Suppose you have 10 line classes that all use the same Point instance. It's a minimal savings.
Mystere Man
But that's even worse, how would you know when to free a Point* if it is referenced by more than one Line? All this trying to treat C++ as a GC language is leading nowhere. I can only agree with zabzonk: In modern C++ there is very seldom a reason to use raw pointers.
mghie
For general use, there seldom a *good* reason to use raw pointers (other than arrays, or pointer arithmetic). But in Frameworks, pointers are often used for a variety of reasons. MFC, VCL, QT, etc all make extensive use of pointers.
Mystere Man
A: 
class Line
{
    private:
        Point *p1; /* No memory allocated */
        Point *p2; /* No memory allocated */
    public:
        Line(Point &p1, Point &p2) : p1(p1), p2(p2) {}

        void setPoints(Point &p1, Point &p2) /* passed references to Point objects */
        {
            this->p1 = p1; /* asssiging Point objects to Point *! */
            this->p2 = p2; /* asssiging Point objects to Point *! */
        }
}

The setPoints() function wouldn't work - at first glance. It should be

            void setPoints(Point &p1, Point &p2) 
            {
                this->p1 = &p1; /* asssiging Point objects to Point *! */
                this->p2 = &p2; /* asssiging Point objects to Point *! */
            }

Then again, we don't have control over when p1 and p2 get destroyed. It's better to create this->p1 and this->p2 using the data in p1 and p2 so that the destructor has control over the memory

alok
+5  A: 

+1 what zabzonk said.

The time to use pointers, as you have used them, would be:

  1. You have several points
  2. You want to create lines using those points
  3. You want to change the values of the points and have the lines changed implicitly.

Step 3 above can be achieved if lines contain pointers to existing points. It introduces complexity though (e.g., "when you destroy a Point instance, what happens to Line instances which contain pointers to the Point?"), which doesn't exist when (as zabzonk suggested) each Line contains its own Point values.

ChrisW
+6  A: 

Whether or not the Point members of your line class are pointers or not creates a very different type of class. Using pointers will result in a classical CoGo style approach, which can be thought of as points being like nails in a board, and lines being rubber bands connecting those nails. Changing a point is like moving a nail, all the associated line works automatically follows, which is desirable in certain applications.

Using literal points means that the lines are all independant of one another, which is appropriate to other types of application.

This is a critical design decision for your classes at this stage.

Edit: As noted in other posts and the comment below, utilising simple pointers to achieve association between multiple lines and points also presents a serious potential problem. Specifically, if a point is deleted or moved in memory, all the pointers referring to that point must be updated. In practice, this tends to be overcome by using unique point IDs to look up a point rather than simple pointers. This also allows the CoGo structures to be easily serialized / saved. Thus our point class would have a static member to get a point based on ID, and our line class would have two point IDs rather than pointers.

Shane MacLaughlin
I have upvoted your answer earlier today, but now I'm not so sure any more: there is much confusion in other answers on when to use pointers, and while I agree with your premise you leave out lifetime management issues of using Point* completely. Maybe you could edit your otherwise excellent answer?
mghie
A: 

The compiler will give errors on this code:

  1. In the initialisation list of the Line constructor, you are assigning Point reference p1 to class member p1, which is a pointer to Point. To get this to compile, you should use Line(Point &p1, Point &p2) : p1(&p1), p2(&p2).
  2. The same problem occurs in the setpoints method. That shoudl be changed to this->p1 = &p1, etc.
  3. Minor issue: after the closing } of the class, put in a semicolon (;)

I think that concludes the syntax.

There is another issue with the code:

The p1 and p2 class members of Line are pointers to Point instances. Who creates these instances and who will delete them when they are not needed anymore? If you design your class as it is now, the code that creates a Line instance passes two Point instances to the constructor. If these Point instances are deleted before the Line is, the Line is left with two dangling pointers (pointers that no longer refer to a valid instance).

Also, the two Point instances that are now part of the Line can be modified from code outside of the Line class. This can lead to very undesirable situations.

In this situation I would declare the p1 and p2 members as Point (instead of a pointer to Point). That way, the Point instances that are passed to the constructor are copied to the class members. As long as the Line exists, the Point members will exist. They can only be changed by the line class itself.

Renze de Waal
A: 

Before asking about the language guru's opinion, start thinking about the design, in this case especially about the lifetime of your objects: does a point need to exist without a line? Do lines share points? Who creates a point, when does it stop to exist? Are two points with the same coordinates identical, or just equal (one might be red, the other might be blue)? ...

It appears that most here agree that you should use value-semantics on such a small code base. Sometimes, the problem requires the same object (i.e. Point) to be referenced by many sides, then use pointers (or references).

The choice between a pointer and a reference is still something else. I prefer using a reference when implementing aggregation (a line can't exist without it's endpoints), and a pointer when implementing a less 'intimate' relation.

xtofl
My description of the difference between a reference and a pointer is at http://stackoverflow.com/questions/448056/c-singleton-getinstance-return/448068#448068
ChrisW
tx. Much discussion about that topic, too.
xtofl
A: 

Use Point objects in your Line class. The ownership of the points is unclear and you risk ending up with dangling pointers or leaked memory.

Your default constructor is a problem as you will rarely want to create a Point at (0,0). You are better off setting the default x,y values to something like MAXINT and asserting that any use of Point doesn't have MAXINT as one of its values. Have an is_valid() function so that clients can test. Your Line class can also have a precondition that its two points are not invalid. The pay off of not allowing a valid point to have a MAXINT value is that you can detect when Points have not been initialized properly, and you'll zap out some otherwise difficult to find bugs in the class usage.

lilburne