views:

658

answers:

13

I have a class (Foo) which lazy loads a property named (Bar). What is your preferred way to protect against mistaken use (due to intellisense or inexperienced staff) of the uninitialized backing field?

I can think of 3 options:

  class Foo {
    // option 1 - Easy to use this.bar by mistake. 
    string bar;
    string Bar {
        get {
            // logic to lazy load bar  
            return bar; 
        }
    }

    // option 2 - Harder to use this._bar by mistake. It is more obscure.
    string _bar2;
    string Bar2 {
        get {
            // logic to lazy load bar2  
            return _bar2;
        }
    }

    //option 3 - Very hard to use the backing field by mistake. 
    class BackingFields {
        public string bar; 
    }

    BackingFields fields = new BackingFields();

    string Bar3 {
        get {
            // logic to lazy load bar  
            return fields.bar;
        }
    }

}

Keep in mind, the only place I want people mucking around with the backing field bar is in setter and getter of the property. Everywhere else in the class they should always use this.Bar

Update

I am currently using the following Lazy implementation (not for all properties with backing fields, but for select ones that require lazy loading, synchronization and notification). It could be extended to support futures as well (force evaluation in a separate thread in a later time)

Note My implementation locks on read, cause it supports an external set.

Also, I would like to mention that I think this is a language limitation which can be overcome in Ruby for example.

You can implement lazy in this way.

x = lazy do
    puts "<<< Evaluating lazy value >>>"
    "lazy value"
end

puts x
# <<< Evaluating lazy value >>>
# lazy value
+1  A: 

I usually just go for option 1. Because it is a private field I don't think it really an issue, and using something like the wrapper class as in your option 3 only makes code difficult to read and understand.

Grzenio
+2  A: 

Option 1, coupled with some education.

Rationale: software is meant to be read more often than written, so optimize for the common case and keep it readable.

Morendil
A: 
// option 4
class Foo
{
    public int PublicProperty { get; set; }
    public int PrivateSetter { get; private set; }
}

C# 3.0 feature, the compiler will generate anonymous private backing fields which can't be accessed by mistake, well unless you use reflection...

EDIT: Lazy instantiation

You can have laziness like this:

// I changed this with respect to ShuggyCoUk's answer (Kudos!)
class LazyEval<T>
{
  T value;
  Func<T> eval;
  public LazyEval(Func<T> eval) { this.eval = eval; }
  public T Eval()
  {
      if (eval == null)
          return value;
      value = eval();
      eval = null;
      return value;
  }
  public static implicit operator T(LazyEval<T> lazy) // maybe explicit
  {
    return lazy.Eval();
  }
  public static implicit operator LazyEval<T>(Func<T> eval) 
  {
    return new LazyEval(eval);
  } 
}

Those implicit conversion make the syntax tidy...

// option 5
class Foo
{
    public LazyEval<MyClass> LazyProperty { get; private set; }
    public Foo()
    {
        LazyProperty = () => new MyClass();
    }
}

And closures can be used to carry scope:

// option 5
class Foo
{
    public int PublicProperty { get; private set; }
    public LazyEval<int> LazyProperty { get; private set; }
    public Foo()
    {
        LazyProperty = () => this.PublicProperty;
    }
    public void DoStuff()
    {
        var lazy = LazyProperty; // type is inferred as LazyEval`1, no eval
        PublicProperty = 7;
        int i = lazy; // same as lazy.Eval()
        Console.WriteLine(i); // WriteLine(7)
    }
}
John Leidegren
Can you explain how you make this property lazy load?
Sam Saffron
No, you can't have lazy instantiation this way. But if you really need laziness you might wanna invest time in something like a generic container for Lazy`1 for lazy instantiation...
John Leidegren
This just seems way too complicated to me - nice code, but you're adding complexity. I think any benefit of forcing users not to use the backing fields is cancelled out by the decreased readability.
Keith
+3  A: 

I usually go for option 2, as it is easier to spot mistakes later on, although option 1 would pass a code review. Option 3 seems convoluted and whilst it may work, it's not going to be nice code to revisit 6 months down the line whilst trying to refactor/fix a bug/etc.

Rowland Shaw
@Rowland, +1, although I'd also add "...and train your inexperienced developers properly so that they know what they're doing and don't break stuff."
LukeH
A: 

Option 4 (a new solution):

See if the question is really about how to prevent people from using an uninitialized variable then init it with an KNOWN INVALID value.

I would say something like:

string str = "SOMETHING_WRONG_HERE";

Who ever is using 'str' will have some sort of warning.

Otherwise Option 3 if preventing users from using 'str' is more important than readability etc.

Sesh
Why do you think anyone using str will get a warning? This kind of code winds up shipping to customers more often than you'd like.
John Saunders
A: 

Automatic properties:

public int PropertyName { get; set; }

will prevent access to the backing field. But if you want to put code in there (e.g. for lazy loading on first access) this clearly won't help.

The simplest route is likely to be a helper type which does the lazy loading, and have an private field of that type, with the public property calling to the correct property/method of the helper type.

E.g.

public class Foo {
  private class LazyLoader {
    private someType theValue;
    public someType Value {
      get {
        // Do lazy load
        return theValue;
      }
    }
  }

  private LazyLoader theValue;
  public someType {
    get { return theValue.Value; }
  }
}

This has the advantage that the backing field is harder to use than the property.

(Another case of an extra level of indirection to solve problems.)

Richard
This does deserve a +1 cause it is a fourth way of doing this and is remarkably safe. Even if some blind coder uses this.teValue.Value they are still safe ... ofcourse it all just a pile of elephants cause now your lazy loader has the original problem
Sam Saffron
+4  A: 

Option 5

Lazy<T>

works quite nicely in several situations, though option 1 should really be just fine for most projects so long as the developers aren't idiots.

Adding [EditorBrowsable(EditorBrowsableState.Never)] to the field won't help if it is private since this logic only kicks in for intellisense generated from metadata rather than the current code (current project and anything done via project references rather than dlls).

Note: Lazy<T> is not thread safe (this is good, there's no point locking if you don't need to) if you require thread safety either use one of the thread safe ones from Joe Duffy or the Parallel Exetensions CTP

ShuggyCoUk
Lazy is really sexy ... I like it quite a lot
Sam Saffron
One little think about this lazy implementation is that its missing a lock, its an easy fix
Sam Saffron
the default implementation shouldn't be locked, only pay for what you need and avoid complex locking protocols. Providing a ThreadSafeLazy<> is fine if that's required though.In some cases multiple creation calls are actually fine anyway so long as it stabilizes after the initial ones
ShuggyCoUk
@Shaggy, this is the implementation I went with http://code.google.com/p/videobrowser/source/browse/branches/big_refactor/MediaBrowser/Library/Util/Lazy.cs it does some basic locking and change notification
Sam Saffron
@sambo99 you implementation takes a lock on every access. I suggest you check out Joe Duffy's (the guy is pretty much *the* .Net concurrecy guru.
ShuggyCoUk
@Shaggy, it has no choice but to lock, I support 'set' on my lazy object so I need to synchronize the read. I kind of dislike the interlockedexchange stuff cause it makes the code much more complicated.
Sam Saffron
Ah yes sorry I totally missed the set. That's really evil :)It is a *very* sensibvle idea to avoid interlocked unless you know what you're doing so sticking with lock is just fine.
ShuggyCoUk
+2  A: 

Code reviews will catch misuse so just go with the most readable. I dislike attempts to work around bad programmers in code, because 1) they don't work, 2) they make it harder for smart programmers to get their work done, and 3) it addresses the symptom rather than the cause of the problem.

marijne
+3  A: 

How about use of ObsoleteAttribute and #pragma - hard to miss it then!

    void Test1()
    {
        _prop = ""; // warning given
    }
    public string Prop
    {
#pragma warning disable 0618
        get { return _prop; }
        set { _prop = value; }
#pragma warning restore 0618
    }
    [Obsolete("This is the backing field for lazy data; do not use!!")]
    private string _prop;
    void Test2()
    {
        _prop = ""; // warning given
    }
Marc Gravell
+1 I never thought of this, but I would never let this ugliness into my code base :)
Sam Saffron
evil but effective :)
ShuggyCoUk
Sorry for the -1, but this is an evil way to do this that no-one should ever do. Nice bit of lateral thinking though.
Keith
+1  A: 

I would just put a large comment block on the top of the class that would look like that:

/************************************************************
* Note: When updating this class, please take care of using *
*       only the accessors to access member data because of *
*       ... (state the reasons / your name, so they can ask *
*       questions)                                          *
*************************************************************/

Usually, just a note like that should be enough, but if this is the same for all the classes in the project, you might prefer to put it in a simple document that you give to programmers working on the project, and everytime you see code that isn't conform, you point them to the document.

Martin
A: 

Currently, I'm in a similar situation. I have a field which should only be used by its property accessor.
I can't use automatic properties, since I need to perform additional logic when the property is set. (The property is not lazy loaded as well).

Wouldn't it be great if a next version of C# would allow something like this:

public class MyClass
{
    public int MyProperty
    {
        int _backingField;

        get
        {
            return _backingField;
        }
        set
        {
            if( _backingField != value )
            {
                _backingField = value;
                // Additional logic
                ...
            }
        }
    }
}

With such construct, the _backingField variable's scope is limited to the property. I would like to see a similar language construction in a next version of C# :)

But, I'm afraid this feature will never be implemented: http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=381625

Frederik Gheysels
Why bother with your own name for the backing field? Why not just have a new language keyword (like value during the set) that's always the same.
Keith
That would be nice as well.
Frederik Gheysels
A: 

This might be overly simple, but why not abstract all the lazy to a base class

 public class LazyFoo{
      private string bar;
      public string Bar{
         get{
             // lazy load and return
         }
         set {
             // set
         }
      }
    }

    public class Foo : LazyFoo{
      // only access the public properties here
    }

I could see the argument that it is unnecessary abstraction, but it is the simplest way I can see to eliminate all access to backing fields.

Mark
A: 

This seems like trying to design-out mistakes that might not happen in the first place, and basically it's worrying about the wrong thing.

I would go with option 1 + comments:

///<summary>use Bar property instead</summary> 
string bar;

///<summary>Lazy gets the value of Bar and stores it in bar</summary>
string Bar {
    get {
        // logic to lazy load bar  
        return bar; 
    }
}

If you do get a developer who keeps using the backing variable then I'd worry about their technical competence.

By all means design to make your code easier to maintain, but try to keep it simple - any rule that you make for yourself here is going to be more hassle than it's worth.

And if you're still really worried about it create an FxCop (or whatever you're using) rule to check for this sort of thing.

Keith