Generic 'temporary instance' class

Question 1

I've been reading aboutWeakPointer andWeakPointer<T> today, and it looks very useful. Rather than just using it as-is though, I decided to write a wrapper around it that covers a common usage case for me.

Code as follows:

public class Temporary<T> where T : class{    Func<T> generator = null;    WeakReference<T> reference = null;    public T Value     {         get         {             return GetValue();         }     }    public Temporary(T value)        : this(value, null)    { }    public Temporary(Func<T> generator)        : this(null, generator)    { }    public Temporary(T value, Func<T> generator)    {        reference = new WeakReference<T>(value);        this.generator = generator;    }    public T GetValue()    {        if (reference == null)            return null;        T res;        if (!reference.TryGetTarget(out res) && generator != null)        {            res = generator();            reference.SetTarget(res);        }        return res;    }}

I've tested this against my use-case and it works as I expect - the garbage collector cleans out items with no active references, and they are re-instantiated at need.

I'm looking for critiques of the implementation and suggestions for improvements/additions.

Question 2

It's pretty good, clean and simple code. Not a lot to critique really. A few minor things:

Some people (myself included) prefer to prefix private fields with_. This way it's easy to see that it's a field rather than a local variable (and gets rid of thethis. in most cases).
You shouldn't have a public property and a public method which do the same thing. How is a programer supposed to know whether to useValue orGetValue? It's not obvious what the difference is or if there is one at all.
Not 100% sure about thenull check inGetValue(). There is no code path wherereference should ever benull. So it would actually indicate a bug if it were the case which you are hiding with this check. Consider removing the check or actually throwing anInvalidOperationException or similar instead. Detecting bugs early is important. Another option would be to look atCode Contracts to state the invariants of that method.

I have started to try and avoidnull checks where possible by making sure that there is always an object. In your case this could be something like this:

public class Temporary<T> where T : class{    private static Func<T> NullGenerator = () => null;    Func<T> generator = NullGenerator;    WeakReference reference = null;    public T Value    {        get        {            return GetValue();        }    }    public Temporary(T value)        : this(value, NullGenerator)    { }    public Temporary(Func<T> generator)        : this(null, generator)    { }    public Temporary(T value, Func<T> generator)    {        if (generator == null)            throw new ArgumentNullException("generator");        reference = new WeakReference(value);        this.generator = generator;    }    private T GetValue()    {        T res;        if (!reference.TryGetTarget(out res))        {            res = generator();            reference.SetTarget(res);        }        return res;    }}

Question 3

I'm not sure aboutNullGenerator, it makes the code simpler, but I also think it makes it less clear. The explicit check againstnull in the original version is very clear.

Question 4

+1 for the null generator and rewriting GetValue() around nulls. Exactly what I would have said. That null check on reference was certainly a bug. Also, could not agree more with item 2.

Question 5

Good answer Chris. Thereference == null check was a leftover from when I was thinking about implementingIDisposable, but it shouldthrow if there's a problem with the reference. I agree with @svick about the NullGenerator, but the rest of your points are excellent.

ChrisWue 20.6k4 gold badges43 silver badges107 bronze badges · Accepted Answer · 2013-12-06 07:47:52Z

It's pretty good, clean and simple code. Not a lot to critique really. A few minor things:

Some people (myself included) prefer to prefix private fields with_. This way it's easy to see that it's a field rather than a local variable (and gets rid of thethis. in most cases).
You shouldn't have a public property and a public method which do the same thing. How is a programer supposed to know whether to useValue orGetValue? It's not obvious what the difference is or if there is one at all.
Not 100% sure about thenull check inGetValue(). There is no code path wherereference should ever benull. So it would actually indicate a bug if it were the case which you are hiding with this check. Consider removing the check or actually throwing anInvalidOperationException or similar instead. Detecting bugs early is important. Another option would be to look atCode Contracts to state the invariants of that method.

I have started to try and avoidnull checks where possible by making sure that there is always an object. In your case this could be something like this:

public class Temporary<T> where T : class{    private static Func<T> NullGenerator = () => null;    Func<T> generator = NullGenerator;    WeakReference reference = null;    public T Value    {        get        {            return GetValue();        }    }    public Temporary(T value)        : this(value, NullGenerator)    { }    public Temporary(Func<T> generator)        : this(null, generator)    { }    public Temporary(T value, Func<T> generator)    {        if (generator == null)            throw new ArgumentNullException("generator");        reference = new WeakReference(value);        this.generator = generator;    }    private T GetValue()    {        T res;        if (!reference.TryGetTarget(out res))        {            res = generator();            reference.SetTarget(res);        }        return res;    }}

I'm not sure aboutNullGenerator, it makes the code simpler, but I also think it makes it less clear. The explicit check againstnull in the original version is very clear.
+1 for the null generator and rewriting GetValue() around nulls. Exactly what I would have said. That null check on reference was certainly a bug. Also, could not agree more with item 2.
Good answer Chris. Thereference == null check was a leftover from when I was thinking about implementingIDisposable, but it shouldthrow if there's a problem with the reference. I agree with @svick about the NullGenerator, but the rest of your points are excellent.

Movatterモバイル変換

Stack Exchange Network

Generic 'temporary instance' class

1 Answer1

You mustlog in to answer this question.

Related

Hot Network Questions

Subscribe to RSS