Krdytkyu

I have a wrapper class:

[Serializable]

// (465 references)

public class Value<T>

{

    private T value;



    [NonSerialized]

    private List<IValueListener<T>> listeners = new List<IValueListener<T>>();



    // (232 references)

    public Value(T value)

    {

        this.value = value;

    }



    // (274 references)

    public T Get()

    {

        return value;

    }



    // (18 references)

    public void Set(T value)

    {

        if (!this.value.Equals(value))

        {

            this.value = value;



            foreach (IValueListener<T> listener in listeners)

            {

                listener.ValueUpdated(this);

            }

        }

    }



    public void AddListener(IValueListener<T> listener)

    {

        listeners.Add(listener);

    }



    public void RemoveListener(IValueListener<T> listener)

    {

        listeners.Remove(listener);

    }

}

The listener interface:

public interface IValueListener<T>

{

    void ValueUpdated(Value<T> source);

}

It's used in a program that alters save files of a game. An object is constructed using the save file data and any value types encountered are wrapped in the Value class. Object is then managed by the program and the save file is rewritten from the objects data upon saving.

The listener is here since altering some data in the save file influences other, so this gives me a way to manage that on a low level (instead of making TextBoxes notify each other).

When I save the file, I actually clone the object, process the clone for saving and save the clone. That's why the class is Serializable. I don't care about listeners when saving so the list of them isn't.

My question is whether what I have done here is a well written code or should it be substituted with something else.

I also copied some of the # of references that VS displays just to demonstrate how heavily this class is used.

You obviously implemented the Observer pattern here.

So let's improve the naming and the overall design so that the pupose of each class and method is clear to the user.

First instead of just Value the class should be called ObservableValue and consequently the interface should be:

public interface IObservableValue<T>

{

    void Subscribe(IObserver<T> observer);

    void Unsubscribe(IObserver<T> observer);

}

the IValueListener interface becomes IObserver:

public interface IObserver<T>

{

    void Notify(T value);

}

Then the new class would look like this:

[Serializable]

public class ObservableValue<T> : IObservableValue<T>

{

    private T _value;



    [NonSerialized]

    private List<IObserver<T>> _observers = new List<IObserver<T>>();



    public ObservableValue(T value)

    {

        _value = value;

    }



    public T Get()

    {

        return _value;

    }



    public void Set(T value)

    {

        if (!_value.Equals(value))

        {

            _value = value;

            NotifyObservers();

        }

    }



    public void Subscribe(IObserver<T> observer)

    {

        _observers.Add(observer);

    }



    public void Unsubscribe(IObserver<T> observer)

    {

        _observers.Remove(observer);

    }



    private void NotifyObservers()

    {

        foreach (var observer in _observers)

        {

            observer.Notify(_value);

        }

    }

}

I used the T as a parameter for the Notify method but if you really need it to be the IObservableValue that it's a minor adjustment.

It is unclear how to use this class if I want to subscribe to two different objects that implement, say, Value<double>. There is no way to tell which of the two values has changed. This looks like a pretty major design flaw, unless I am missing something. So you should probably either use regular events (so I can supply different handlers for different values) or provide "source" argument. Personally, I think first option is better. You should also consider using proper event aggregator instead of storing the list of listeners in values themselves.

Also I think you should have an actual property instead of Get() and Set() methods. And you can make listeners field read-only.

As the type seems to be used often, note that the observers/listeners have to be unsubscribed to avoid memory leaks. If that is not easy possible, one option is to store the observers/listeners as weak references so that the GC collects them if they are not references anywhere else.

[...]



[NonSerialized]

private List<WeakReference<IObserver<T>>> _observers = new List<WeakReference<IObserver<T>>>();



[...]



public void Subscribe(IObserver<T> observer)

{

    _observers.Add(new WeakReference<IObserver<T>>(observer));

}



[...]



private void NotifyObservers()

{

    IObserver<T> observer;

    foreach (var observerRef in _observers)

    {  

        if (observerRef.TryGetTarget(out observer))

        {

            observer.Notify(_value);

        }

    }

}