My copy constructor isn't working

Question

Problem has been fixed! Thank you!

My copy constructor doesn't seem to work (I've tried the same constructor I've used for a previous project):

public Rectangle(Rectangle Rectangle)
{
    bottomleft = Rectangle.bottomleft;
    topright = Rectangle.topright;
}

basically I have a class called Rectangle, that takes two points (bottomleft point and topright point - the rectangle is parallel/perpendicular to the X/Y axis).

I tested this copy constructor by creating a new rectangle, and moving the new rectangle's coordinates by 3. however when I did .toString() to the first rectangle and the second, the coordinates on both were shifted, rather than just the second rectangle!

Here's what I did:

Point bottom = new Point(0, 0); // a class that stores two 'double' objects (double x, double y)
Point top = new Point(5, 5);
Rectangle first = new Rectangle(bottom,top); // creates new rectangle, horizontal lines are parallel to X axis, vertical are parallel to Y axis

Rectangle second = new Rectangle(first); // attempt to use copy constructor
second.Move(3, 3); // moves the second rectangle three units up and three units to the right
Console.WriteLine(first.ToString()+"\n"+second.ToString()); // posts the coordinates of the two rectangles

The expected output would be (0,0),(5,5) for the first rectangle, and (3,3)(8,8) for the second, but instead the output is (3,3),(8,8) for both rectangles, meaning that both were changed.

How do I fix my copy constructor (posted above)?

Problem has been fixed! Thank you! As per common suggestion, I used a copy constructor on the 'Point' class as well. I'm leaving this question in case it helps someone in the future.

My Point class has a copy constructor of it's own (nearly identical to the one I posted above), and I modified my Rectangular constructor to be like this:

public Rectangle(Rectangle Rectangle) 
    {
        Point a = new Point(Rectangle.GetBottomPoint());
        Point b = new Point(Rectangle.GetTopPoint());
        this.bottomleft = a;
        this.topright = b;
    }

if point is class, you should copy points too. by creating new point, `new Point(x,y)` — M.kazem Akhgary, Sep 30 '17 at 11:33
The `public Rectangle(Rectangle Rectangle)` line is so C#... Is your `Point` a class? If yes, clone them too or use a struct (for example, `System.Drawing.PointF`) — , Sep 30 '17 at 11:34
Use type `SystemDrawing.Point` which is struct and will be copied by values. Where you version of `Point` is a class, and and copied to another rectangle by reference - so both rectangles reference to same points — Fabio, Sep 30 '17 at 11:39
If you want to use your own `Point` class then implement CopyConstructor for that class too and use it. — MKR, Sep 30 '17 at 11:42
Using a `struct` for your points properties is the right way, this will force you to use different points inside your class, rather than reusing the same point *instance*. — Federico Dipuma, Sep 30 '17 at 11:43
Show us the full definition of Rectangle (especially `Move`) and of `Point`. Otherwise it’s just random guessing here. If done properly, `Rectangle` and `Point` should be structs, and `Move` should return the new rectangle after moving it. — poke, Sep 30 '17 at 12:01
A copy constructor is a C++ concept, it does not exist in C#. In C++ everything behaves like a value type, references need to be explicitly declared and there is no fundamental difference between a struct and a class. Not the C# way. You need to at least make Point a struct instead of a class so it behaves like a value instead of an object. — Hans Passant, Sep 30 '17 at 12:09
@M.kazemAkhgary ,someone ,MKR - Thank you! This indeed fixed the problem - I didn't realize that the coordinates in the new rectangles would still be referring to the same Points. I'd upvote all three of you, but I don't have enough rep to upvote comments — xland44, Sep 30 '17 at 15:09
Indeed, mathematical structures like points and rectangles are best modeled as *immutable value types*. You don't change the corners of a rectangle; you make a new rectangle with the corners you want. Just like you don't change the number 12 to 13 when you add 1 to it; 12 stays 12; 13 is a different number. — Eric Lippert, Sep 30 '17 at 15:52

InBetween · Answer 1 · 2017-09-30T15:16:11.420

If you make Point and Rectangle value types (struct) you don't need copy constructors at all:

struct Point { .... }

var p1 = new Point(...);
var p2 = p1; //p2's value is a copy of p1's value
Frob(ref p2);
p2.IsFrobbed; //true
p1.IsFrobbed; //false

Variables in c# are copied by value, that means that when you see an assignment expression of the type var p2 = p1 a copy of the value stored in p1 is made and stored in p2.

The value stored in value type variable is the value type instance itself, therefore p2 = p1 creates a new identical instance of the one stored in p1 and stores it in p2.

However, if its reference type variable, the stored value is the address or reference in memory where the instance resides (hence the name reference type). So, when you do var p2 = p1 you are copying the address of the instance, not the instance itself; thus both p1 and p2 end up referencing the same instance and the behavior changes radically:

class Point { .... }

var p1 = new Point(...);
var p2 = p1; //p2's value is a copy of p1's value
Frob(p2);
p2.IsFrobbed; //true
p1.IsFrobbed; //true!

This is your issue, Point is a reference type, you should probably make it a value type (read MS's recommendations or this SO question and this one on when you should implement a type as a value type or a reference type) or implement a copy mechanism like you are doing in Rectangle.

but you should not encourage using struct, for beginners, this answer means to them, use struct when you want to copy stuff. but that's totally wrong. though this answer is correct, but its misleading for beginners. — M.kazem Akhgary, Sep 30 '17 at 11:59
@M.kazemAkhgary I wasn't finished ;). I was meaning to add a link with guidelines on when to implement any given type as a value type or reference type. — InBetween, Sep 30 '17 at 12:02
The first code example cannot possibly work as stated if `Point` is a `struct`. You are passing `p2` to `Frob` by value, meaning that whatever `Frob` does will modify a *copy* of `p2`. — Kirill Shlenskiy, Sep 30 '17 at 15:14

My copy constructor isn't working

1 Answers1