Is private set dangerous?

  • Thread starter Thread starter Breck
  • Start date Start date
B

Breck

When I began work tonight, my class looked like this:

public class Plane3D
{
Vector3D _normal;
double _constant;

public Vector3D Normal { get { return _normal; } }
public double Constant { get { return _constant; } }

public Plane3D(Vector3D normal, double constant)
{
_normal = normal;
_normal.Normalize();

this._constant = constant;
}

public Plane3D(Vector3D p0, Vector3D p1, Vector3D p2)
{
_normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);
_normal.Normalize();

_constant = Vector3D.DotProduct(_normal, p0);
}

public double DistanceTo(Vector3D p)
{
return Math.Abs(Vector3D.DotProduct(_normal, p) - _constant);
}
}

Then I Installed ReSharper (JetBrain's Visual Studio plugin), and its
refactoring tools suggested I change the fields to automatic
properties with private setters:

public class Plane3D
{
public Vector3D Normal { get; private set; }
public double Constant { get; private set; }

public Plane3D(Vector3D normal, double constant)
{
Normal = normal;
Normal.Normalize();

this.Constant = constant;
}

public Plane3D(Vector3D p0, Vector3D p1, Vector3D p2)
{
Normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);
Normalize();

Constant = Vector3D.DotProduct(Normal, p0);
}

public double DistanceTo(Vector3D p)
{
return Math.Abs(Vector3D.DotProduct(Normal, p) - Constant);
}
}

After this change, though, the calls to Normal.Normalize() leave the
field Normal unchanged. My ignorance of private set before tonight
should indicate I'm a C# nublet, but this struck me as egregiously
dangerous because it compiled without warnings. Is there a best
practice regarding private set I need to know about, or do I have a
more fundamental misunderstanding of how properties work?

Thanks in advance,
-- Breck
 
Note that in the second, revised, constructor the call is actually
Normal.Normalize() - I made a mistake in copying my code when I posted
it.
 
Breck said:
[...]
After this change, though, the calls to Normal.Normalize() leave the
field Normal unchanged. My ignorance of private set before tonight
should indicate I'm a C# nublet, but this struck me as egregiously
dangerous because it compiled without warnings. Is there a best
practice regarding private set I need to know about, or do I have a
more fundamental misunderstanding of how properties work?
Click to expand...

Well, you didn't give us enough information to know for sure what
"Vector3D" is. But, let me guess. It's either
System.Windows.Media.Media3D.Vector3D, or another value type (i.e.
"struct") like that type.

There are at least two big mistakes here, neither of them yours (you did
make a mistake, but it was a little one :) ). And no, this has
_nothing_ at all to do with the setter being private.

First, IMHO Resharper should have not suggested you change from
accessing a private field to always going through the property without
warning you about the semantic change when the field/property type is a
value type.

Second, Microsoft should stop setting this bad example of having mutable
value types. This is exactly the kind of subtle, hard-to-notice bug
that happens when a value type is mutable.

So, with that said, here's what I think is going on (and for sure, is
what's going on if Vector3D is a value type):

-- when you called "_normal.Normalize()", that accessed the field
directly, allowing the Normalize() method to modify the value in that field

-- after the change to use the property, instead of accessing the
backing field directly, the property returns a _copy_ of the value
stored in the backing field. It's this copy that the Normalize() method
acts on, leaving the original background field unchanged.

This is a consequence of the specific behavior of value types. That is,
you are always operating on a specific copy of the value type, and
unless you are accessing the copy directly from a variable, the instance
of the value is a copy retrieved from somewhere else, and that somewhere
else is no longer in any way attached to the instance you're operating
on. That's what value types do; they get copied rather than referenced.

You can either go back to using the field directly, or you can rewrite
the code like this (for example):

public Plane3D(Vector3D normal, double constant)
{
normal.Normalize();
Normal = normal;

this.Constant = constant;
}

public Plane3D(Vector3D p0, Vector3D p1, Vector3D p2)
{
Vector3D normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);

normal.Normalize();
Normal = normal;

Constant = Vector3D.DotProduct(Normal, p0);
}

Note that if Vector3D had been a proper, immutable value type, you'd
always need an assignment to the Normal property to change its value
anyway. So even though following the above approach might seem
unwieldy, it is in fact the correct way to deal with value types and is
only slightly less simple than if Vector3D had been immutable.

(If it had been immutable, the Normalize() method would return a new
value, and so this code:


Vector3D normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);

normal.Normalize();
Normal = normal;

would instead look like this:


Normal = Vector3D.CrossProduct(p1 - p0, p2 - p0).Normalize();

which IMHO is actually a lot nicer than either of the syntaxes you're
stuck with as things are now).

Anyway, it's up to you. I'm not sure there's much value in this
particular example of using an auto-property rather than explicitly
having your own backing field. On the other hand, a) using the
auto-property gives you extra assurance the backing field won't change
without going through the property, and b) in this particular example it
looks like the property itself is basically supposed to be immutable, so
you shouldn't find yourself writing a lot of "copy to variable, change,
copy back to property" code.

So, six of one, half dozen of the other in this particular case. :)

Pete
 
After this change, though, the calls to Normal.Normalize() leave the
field Normal unchanged. My ignorance of private set before tonight
should indicate I'm a C# nublet, but this struck me as egregiously
dangerous because it compiled without warnings. Is there a best
practice regarding private set I need to know about, or do I have a
more fundamental misunderstanding of how properties work?
Click to expand...

I am not sure why Normalize() is failing, but there is nothing wrong
with a private set. If the item is only to be set from the constructor,
it is the proper method.

As you do not have the code for Vector3D showing, I am not sure what is
happening.

The best method is to set up a unit test that has the expected outcome.
Run it and it fails, then debug and step through. You may have to put
some Debug.Asserts in the code or breakpoints in Normalize.

As it stands, I don't have enough information in the post to tell you
why you are having an issue, but the private set is NOT the problem from
what I see. And I doubt very seriously it could be a problem in any case
like this.

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
I am not sure why Normalize() is failing, but there is nothing wrong
with a private set. If the item is only to be set from the constructor,
it is the proper method.

As you do not have the code for Vector3D showing, I am not sure what is
happening.

The best method is to set up a unit test that has the expected outcome.
Run it and it fails, then debug and step through. You may have to put
some Debug.Asserts in the code or breakpoints in Normalize.

As it stands, I don't have enough information in the post to tell you
why you are having an issue, but the private set is NOT the problem from
what I see. And I doubt very seriously it could be a problem in any case
like this.
Click to expand...

About the only thing I could see is if Vector3D is a immutable object... And
the problem would have been there even in the original.
 
Breck said:
[...]
After this change, though, the calls to Normal.Normalize() leave the
field Normal unchanged. My ignorance of private set before tonight
should indicate I'm a C# nublet, but this struck me as egregiously
dangerous because it compiled without warnings. Is there a best
practice regarding private set I need to know about, or do I have a
more fundamental misunderstanding of how properties work?
Click to expand...

Well, you didn't give us enough information to know for sure what
"Vector3D" is.  But, let me guess.  It's either
System.Windows.Media.Media3D.Vector3D, or another value type (i.e.
"struct") like that type.

There are at least two big mistakes here, neither of them yours (you did
make a mistake, but it was a little one :) ).  And no, this has
_nothing_ at all to do with the setter being private.

First, IMHO Resharper should have not suggested you change from
accessing a private field to always going through the property without
warning you about the semantic change when the field/property type is a
value type.

Second, Microsoft should stop setting this bad example of having mutable
value types.  This is exactly the kind of subtle, hard-to-notice bug
that happens when a value type is mutable.

So, with that said, here's what I think is going on (and for sure, is
what's going on if Vector3D is a value type):

    -- when you called "_normal.Normalize()", that accessed the field
directly, allowing the Normalize() method to modify the value in that field

    -- after the change to use the property, instead of accessing the
backing field directly, the property returns a _copy_ of the value
stored in the backing field.  It's this copy that the Normalize() method
acts on, leaving the original background field unchanged.

This is a consequence of the specific behavior of value types.  That is,
you are always operating on a specific copy of the value type, and
unless you are accessing the copy directly from a variable, the instance
of the value is a copy retrieved from somewhere else, and that somewhere
else is no longer in any way attached to the instance you're operating
on.  That's what value types do; they get copied rather than referenced..

You can either go back to using the field directly, or you can rewrite
the code like this (for example):

   public Plane3D(Vector3D normal, double constant)
   {
     normal.Normalize();
     Normal = normal;

     this.Constant = constant;
   }

   public Plane3D(Vector3D p0, Vector3D p1, Vector3D p2)
   {
     Vector3D normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);

     normal.Normalize();
     Normal = normal;

     Constant = Vector3D.DotProduct(Normal, p0);
   }

Note that if Vector3D had been a proper, immutable value type, you'd
always need an assignment to the Normal property to change its value
anyway.  So even though following the above approach might seem
unwieldy, it is in fact the correct way to deal with value types and is
only slightly less simple than if Vector3D had been immutable.

(If it had been immutable, the Normalize() method would return a new
value, and so this code:

     Vector3D normal = Vector3D.CrossProduct(p1 - p0, p2 - p0);

     normal.Normalize();
     Normal = normal;

would instead look like this:

     Normal = Vector3D.CrossProduct(p1 - p0, p2 - p0).Normalize();

which IMHO is actually a lot nicer than either of the syntaxes you're
stuck with as things are now).

Anyway, it's up to you.  I'm not sure there's much value in this
particular example of using an auto-property rather than explicitly
having your own backing field.  On the other hand, a) using the
auto-property gives you extra assurance the backing field won't change
without going through the property, and b) in this particular example it
looks like the property itself is basically supposed to be immutable, so
you shouldn't find yourself writing a lot of "copy to variable, change,
copy back to property" code.

So, six of one, half dozen of the other in this particular case.  :)

Pete
Click to expand...

Pete,

You nailed it. I was using System.Windows.Media.Media3D.Vector3D,
which is a struct and therefore a value type. I misunderstood what
properties were, which is simply syntactic sugar for actual getter and
setter methods, which have all the nuances of passing references and
value types by reference and by value.

Thanks for your help,
-- Breck
 
Breck said:
You nailed it. I was using System.Windows.Media.Media3D.Vector3D,
which is a struct and therefore a value type. I misunderstood what
properties were, which is simply syntactic sugar for actual getter and
setter methods, which have all the nuances of passing references and
value types by reference and by value.
Click to expand...

Just FYI: properties aren't just "syntactic sugar". They are a real
member of a type, different from other members. It's true that they
encapsulate a setter and a getter, and there's arguably some "sugar" in
the syntax used to call those methods, but they are full-fledged type
components, having all the run-time visibility that actual "syntactic
sugar" would not have.

Pete
 
Breck wrote:
Click to expand...

I second what Pete DUmbo says, as he's often right.

You have to be careful between a reference and a value object. A
structure (like a Point, or a primitive data type like an int32) is a
value object, while an array (or a class) is a reference object. This
is very hard to describe, but you have to really study a long time to
appreciate the difference. Most of the time, it does not matter, but
sometimes it does. That's as good as I can do for such a short
entry. Trust me on this--you have to study it, that's all. No
shortcuts. Like delegates and fucntion pointers (C++)--you just have
to study it until your eyeballs explode, until you lern it.

RL
 
Back
Top