views:

1045

answers:

19

Sometimes you have a private field that backs a property, you only ever want to set the field via the property setter so that additional processing can be done whenever the field changes. The problem is that it's still easy to accidentally bypass the property setter from within other methods of the same class and not notice that you've done so. Is there a way in C# to work around this or a general design principle to avoid it? Thanks.

+5  A: 

There is no such provisioning in C#.

However I would name private variables differently (e.g. m_something or just _something) so it is easier to spot it when it is used.

Josip Medved
The _ prefix for private variables is so common that I would be surprised if it was used for "more private" variables. Something more awkward should get the attention of those new to this code base.
Malte Clasen
+18  A: 

There's no inbuilt way to do what you want to do, but by the sounds of things you need another layer of abstraction between your class and that value.

Create a separate class and put the item in there, then your outer class contains the new class, and you can only access it through it's properties.

Binary Worrier
A: 

As a design practice, you could use a naming convention for "private properties" that's different from normal public members - for instance, using m_ItemName for private items instead of ItemName for public ones.

Amber
+2  A: 

C# has no language feature for this. However, you can rely on naming conventions, similar to languages which have no private properties at all. Prefix your more private variable names with _p_, and you'll be pretty sure that you don't type it accidentally.

Malte Clasen
A: 

If you're using the C# 3.0 compiler you can define properties which have compiler-generated backing fields like this:

public int MyInt { get; set; }

That will mean there is only one way to access the property, sure it doesn't mean you can only access the field but it does mean that there's nothing but the property to access.

Slace
In question he mentioned that additional processing needs to be done. In that case you cannot use auto properties.
Josip Medved
But with that way no additional functionality can be set into the get/set methods as the TS needs to.
BeowulfOF
+10  A: 

No, there isn't. I'd quite like this myself - something along the lines of:

public string Name
{
    private string name; // Only accessible within the property
    get { return name; /* Extra processing here */ }
    set { name = value; /* Extra processing here */ }
}

I think I first suggested this about 5 years ago on the C# newsgroups... I don't expect to ever see it happen though.

There are various wrinkles to consider around serialization etc, but I still think it would be nice. I'd rather have automatically implemented readonly properties first though...

Jon Skeet
"automatically implemented readonly properties"? Like `public string Name { get; }`? How would that work?
Blorgbeard
No, it would be public readonly string Name { get; set; }. Works just like a normal auto property but the setter only works from a constructor, just like a normal readonly field.
Paul Batum
you're allowed to assign to it in the constructor only.I prefer the syntax `public string Name { get; private readonly set; }`. IT is a little more verbose but clearer in intent.
ShuggyCoUk
Oh *right*, of course. That's much more sensical.
Blorgbeard
@Jon, I asked for this a long time ago, but got told it was not inportant enough to provide. Mybe a email from you to the C# team will get a better result.
Ian Ringrose
@Ian: Possibly... although I may have expressed this desire to them already :) (I'm still hoping for readonly auto-implemented properties for C# 5...)
Jon Skeet
+15  A: 

I'd consider this a nasty hack and try to avoid it if possible, but...

You can mark the backing field as obsolete so that the compiler will generate a warning when you try to access it, and then suppress that warning for the property getter/setter.

The warning codes that you'd need to suppress are CS0612 for the plain Obsolete attribute and CS0618 if the attribute has a custom message.

[Obsolete("Please don't touch the backing field!")]
private int _backingField;

public int YourProperty
{
    #pragma warning disable 612, 618
    get { return _backingField; }
    set { _backingField = value; }
    #pragma warning restore 612, 618
}
LukeH
No offence, but if that was discovered in a code review, the coder should be brough out and shot. As hacks go it is nice though :)
Binary Worrier
@Binary Worrier: Absolutely agree. I think technical solutions to these sort of problems aren't really the answer. The real answer should be "train your developers properly to ensure that they don't hack code that they don't understand".
LukeH
Impressive. I like it.
Jonathan
I would prefer unit testing over hacks to detect misuse.
tvanfosson
A horrible way to define a field. I mean - it's funny to look at, but who votes for such a thing??
Stefan Steinegger
@Stefan: I'm not sure why people would vote for this either, not that I'm complaining! The answer was intended to be along the lines of "just out of interest here's a hacky workaround", *not* "here's what I recommend you do in your production code".
LukeH
Well I suppose it does (sort of) answer the question. On a side note if you use Obsolete("message", true) C# will refuse to compile outside the #pragma blocks.
KeeperOfTheSoul
Clever, but so, so wrong.
Chris Needham
@KeeperOfTheSoul: If you use "`Obsolete("Message", true)`" then C# will refuse to compile what's *inside* the `#pragma` block as well. The `#pragma` only disables warnings, not errors.
LukeH
I like this, as it is very claer from the code what is going on, and noone will bypass this by error
Ian Ringrose
+2  A: 

I don't know C# but in Java you may have a base class with only private instance variables and public setters and getters (should return a copy of the instance var.) and do all other in an inherited class.

A "general design principle" would be "use inheritance".

PeterMmm
I like this approach! And I could even see the logic behind it. I.e.: Buick.Engine should not care how the parent Car handles Car._engine.
Ogre Psalm33
but you can only have 1 base class...
Ian Ringrose
+3  A: 

Perhaps a property backing store, similar to the way WPF stores properties?

So, you could have:

Dictionary<string,object> mPropertyBackingStore = new Dictionary<string,object> ();

public PropertyThing MyPropertyThing
{
    get { return mPropertyBackingStore["MyPropertyThing"] as PropertyThing; }
    set { mPropertyBackingStore["MyPropertyThing"] = value; }
}

You can do all the pre-processing you want now, safe in the knowledge that if anyone did access the variable directly, it would have been really really hard compared to the property accessor.

P.S. You may even be able to use the dependency property infrastructure from WPF...
P.P.S. This is obviously going to incur the cost of casting, but it depends on your needs - if performance is critical, perhaps this isn't the solution for you.
P.P.P.S Don't forget to initialise the backing store! (;

EDIT:

In fact, if you change the value property stored to a property storage object (using the Command pattern for example), you could do your processing in the command object...just a thought.

Kieron
+1  A: 

There is no build in solution in C#, but I think your problem can be solved by good OO design: Each class should have a single purpose. So try to extract the logic around your field into a class as small as possible. This reduces the code where you can access the field by accident. If you do such errors by accident, your class is probably to big.

Often interface are good to restrict access to only a certain "subset" of an object. If that's appropriate for your case depends on your setting of course. More details about the work to be done would help to provide a better answer.

Achim
+5  A: 

You can put all of your private fields into a nested class and expose them via public properties. Then within your class, you instantiate that nested class and use it. This way those private fields are not accessible as they would have been if they were part of your main class.

public class A
{
   class FieldsForA
   {
      private int number;
      public int Number
      {
         get
         {
           //TODO: Extra logic.
           return number;
         }
         set
         {
           //TODO: Extra logic.
           number = value;
         }
      }
   } 
   FieldsForA fields = new FieldsForA();

   public int Number
   {
      get{ return fields.Number;}
      set{ fields.Number = value;}
   }       
}

It just provides a level of obstruction. The underlying problem of accessing private backing fields is still there within the nested class. However, the code within class A can't access those private fields of nested class FieldForA. It has to go through the public properties.

Mehmet Aras
+1  A: 

You say that you do additional processing. Presumably this would be detectable under the correct conditions. My solution, then, would be to create unit tests that implement conditions such that if the backing field is used directly the test will fail. Using these tests you should be able to ensure that your code correctly uses the property interface as long as the tests pass.

This has the benefit that you don't need to compromise your design. You get the safety of the unit tests to ensure that you don't accidently make breaking changes and you capture the understanding of how the class works so that others who come along later can read your tests as "documentation."

tvanfosson
I don't think this will work, as the programmer that adds the new method that bypasses the property is not lickly to extend the test to check the property is not bypassed...
Ian Ringrose
+33  A: 

IMHO, it is not used, because:

  • The class must trust itself
  • If your class gets as large that one part does not know the other, it should be divided.
  • If the logic behind the property is slightly more complex, consider to encapsulate it in an own type.
Stefan Steinegger
Sounds good, but that often compromises a consistent, straightforward interface, or requires a lot of boilerplate code, i.e. decreasing maintainability.
peterchen
+1, @peterchen: You could create a facade to replace the class, thus calling all of the other classes that this one was split into.
SnOrfus
+7  A: 

You CAN do this, by using a closure over a local in the constructor (or other initialisation function). But it requires significantly more work that the helper class approach.

class MyClass {
  private Func<Foo> reallyPrivateFieldGetter;
  private Action<Foo> reallyPrivateFieldSetter;
  private Foo ReallyPrivateBackingFieldProperty {
    get { return reallyPrivateFieldGetter(); }
    set { reallyPrivateFieldSetter(value); }
  }

  public MyClass() {
    Foo reallyPrivateField = 0;
    reallyPrivateFieldGetter = () => { return reallyPrivateField; }
    reallyPrivateFieldSetter = v => { reallyPrivateField = v; };
  }
}

I suspect that the underlying field type Foo will need to be a reference class, so the two closures are created over the same object.

Richard
Argh, went to paste in my own version of this particular solution and found you beat me to it! +1
Paul Batum
this has a runtine overhead
Ian Ringrose
A: 

I agree with the general rule that the class should trust itself (and by inference anybody coding within the class). It is a shame that the field is exposed via intellisense.
Sadly placing [EditorBrowsable(EditorBrowsableState.Never)] does not work within that class (or indeed the assembly(1))

In Visual C#, EditorBrowsableAttribute does not suppress members from a class in the same assembly.

If you really do wish to solve this aspect of it the the following class may be useful and makes the intent clear as well.

public sealed class TriggerField<T>
{
    private T data;

    ///<summary>raised *after* the value changes, (old, new)</summary>
    public event Action<T,T> OnSet;

    public TriggerField() { }

    ///<summary>the initial value does NOT trigger the onSet</summary>
    public TriggerField(T initial) { this.data=initial; }

    public TriggerField(Action<T,T> onSet) { this.OnSet += onSet; }

    ///<summary>the initial value does NOT trigger the onSet</summary>
    public TriggerField(Action<T,T> onSet, T initial) : this(onSet) 
    { 
        this.data=initial;
    }

    public T Value
    {
        get { return this.data;}
        set 
        { 
            var old = this.data;
            this.data = value;
            if (this.OnSet != null)
                this.OnSet(old, value);
        }
    }
}

Allowing you to (somewhat verbosely) use it like so:

public class Foo
{
    private readonly TriggerField<string> flibble = new TriggerField<string>();
    private int versionCount = 0;

    public Foo()
    {
        flibble.OnSet += (old,current) => this.versionCount++;  
    }

    public string Flibble 
    { 
        get { return this.flibble.Value; }
        set { this.flibble.Value = value; }
    }
}

alternatively you can go for a less verbose option but accessing Flibble is by the not idiomatic bar.Flibble.Value = "x"; which would be problematic in reflective scenarios

public class Bar
{
    public readonly TriggerField<string> Flibble;
    private int versionCount = 0;

    public Bar()
    {
        Flibble = new TriggerField<string>((old,current) => this.versionCount++);
    }
}


  1. or solution if you look at the community content!
ShuggyCoUk
Personally I'd use a simple delegate rather than an event, I'd also have both a pre and post delegate in case you want to prevent changing the field if its invalid (e.g. a property that shouldn't contain more than 10)
KeeperOfTheSoul
I feel the delegate taking constructor covers that use case. But making it an event allows you to swap the behaviour in a reasonable fashion as well as stack operations simply.Pre and post is probably reasonable if you're willing to tolerate exceptions for unreasonable input with recoverability and might include it if I used this myself (I don't) but feel the concept rather than the specific implementation is what I'm trying to get across...
ShuggyCoUk
A: 

Use the "veryprivate" construct type

Example:

veryprivate void YourMethod()
{

  // code here
}
shanabus
+1  A: 

Wrap it in a class? The property thing is a bit like that anyway, associating data with methods - the "Encapsulation" they used to rave about...

class MyInt
{
    private int n;

    public static implicit operator MyInt(int v) // Set
    {
        MyInt tmp = new MyInt();
        tmp.n = v;
        return tmp;
    }

    public static implicit operator int(MyInt v) // Get
    {
        return v.n;
    }
}

class MyClass
{
    private MyInt myint;

    public void func()
    {
        myint = 5;
        myint.n = 2; // Can't do this.
        myint = myint + 5 * 4; // Works just like an int.
    }
}

I'm sure I'm missing something? It seems too normal...

BTW I do like the closures one, superbly mad.

JonB
+2  A: 

Can’t do this in standard C#, however you could

  • define a custom attribute say “OnlyAccessFromProperty”

  • write your code like

    [OnlyAccessFromProperty(Name)] String name

    Name { get{return name;} }

    etc …

  • Then write a custom rule for FxCop (or another checker)

  • Add FxCop to your build system so if your custom rule find an error the build is failed.

Do we need a set of standard custom rules/attributes to enforce common design patens like this without the need to extend C#

Ian Ringrose
A: 

The new Lazy class in .net 4.0

provides support for several common patterns of lazy initialization

In my experience this is the most common reason I wish to wrap a field in a private properly, so solves a common case nicely. (If you are not using .Net 4 yet you can just create your own “Lazy” class with the same API as the .Net 4 version.)

See this and this and this for details of using the Lazy class.

Ian Ringrose