HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Listening for values changed by the memory

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theforlisteningmemoryvalueschanged

Problem

I'm fetching some values from the memory. I would like to create some custom events that I can listen to for each value. Since there are no existing events regarding memory changes, I've decided to go with a polling solution. I'm restarting the timer below manually just to make certain the work is finished.

Would this be an okay solution (considering the circumstances) to my problem? Do you see any issues with creating multiple timers? As far as I know, each instance will run on its own thread.

This was the best I could come up with right off the bat. I would like to hear your input regarding possible improvements:

var vc = new ValueChange(1000, Blah.GetMemoryValue());
vc.PropertyChanged += OnPropertyChanged;

void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
    Console.WriteLine("prop change");
}

public class ValueChange : INotifyPropertyChanged
{
    private int _val;
    private readonly System.Timers.Timer _timer;

    private int Value
    {
        set
        {
           _val = value;
            RaisePropertyChanged("Val");
        }
        get
        {
            return _val;
        }
   }

   public ValueChange(double polingInterval, int val)
   {
        _val = val;

       _timer = new System.Timers.Timer { AutoReset = false, Interval = polingInterval };
       _timer.Elapsed += TimerElapsed;
       _timer.Start();
   }

   private void TimerElapsed(object sender, ElapsedEventArgs e)
   {
       var newValue = Blah.GetMemoryValue();

       if (Value != newValue)
       {
           Value = newValue;
       }

        _timer.Start();

   }

   public event PropertyChangedEventHandler PropertyChanged;

    private void RaisePropertyChanged(string caller)
    {
       if (PropertyChanged != null)
       {
           PropertyChanged(this, new PropertyChangedEventArgs(caller));
       }
    }
}

Solution

-
Having a private property is quite unusual. It's especially weird since you only use it once in TimerElapsed. I would probably just use the field directly and call RaisePropertyChanged() manually when necessary.

-
Why is the initial value passed in the constructor? I think the class should retrieve the initial value by itself.

-
PropertyChanged should be raised for a property that's readable from the outside and the name in the event args should be the same as the name of that property.

-
Your code is a prime candidate for making it more generic: you could change ValueChange to ValueChange and int _val to T _val. (But if you do this, be careful with equality, !object.Equals(_val, newValue) is probably the simplest way to do it correctly.)

Also, Blah.GetMemoryValue should be configurable by the caller. You could achieve this by accepting a Func delegate in the constructor. Something like:

private readonly Func _getValue;

public ValueChange(double polingInterval, Func getValue)
{
    _getValue = getValue;

     …
}

private void TimerElapsed(object sender, ElapsedEventArgs e)
{
    var newValue = _getValue();

    …
}


-
I like to avoid null checks on events by making sure there always is at least one subscriber:

public event PropertyChangedEventHandler PropertyChanged = delegate { };


But this is not really a common technique.

-
Creating multiple timers is okay, they execute on thread pool threads. This means that if several timers tick at the same time, they will be (most likely) executed concurrently by different threads.

-
In general, polling like this is not ideal. It would be better if you could get a notification whenever the value actually changes. But it seems like this is not possible in your case.

-
You shouldn't shorten the names of your variables. So, _value is better than just _val.

Code Snippets

private readonly Func<T> _getValue;

public ValueChange(double polingInterval, Func<T> getValue)
{
    _getValue = getValue;

     …
}

private void TimerElapsed(object sender, ElapsedEventArgs e)
{
    var newValue = _getValue();

    …
}
public event PropertyChangedEventHandler PropertyChanged = delegate { };

Context

StackExchange Code Review Q#35845, answer score: 3

Revisions (0)

No revisions yet.