5
\$\begingroup\$

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));
       }
    }
}
\$\endgroup\$
3
\$\begingroup\$
  1. 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.

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

  3. 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.

  4. Your code is a prime candidate for making it more generic: you could change ValueChange to ValueChange<T> 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<T> delegate in the constructor. Something like:

    private readonly Func<T> _getValue;
    
    public ValueChange(double polingInterval, Func<T> getValue)
    {
        _getValue = getValue;
    
         …
    }
    
    private void TimerElapsed(object sender, ElapsedEventArgs e)
    {
        var newValue = _getValue();
    
        …
    }
    
  5. 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.

  6. 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.

  7. 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.

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

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the feedback - much appreciated! I do have a couple of questions though. Could you show me how to pass a Func<T> to the constructor, and make use of it in the timer event? I can't get it to work. \$\endgroup\$ – Johan Nov 21 '13 at 19:32
  • \$\begingroup\$ @Johan See edit (it assumes you already successfully changed the class to T instead of int). \$\endgroup\$ – svick Nov 21 '13 at 19:35
  • \$\begingroup\$ Could you show me an example when instantiating ValueChange? \$\endgroup\$ – Johan Nov 21 '13 at 19:41
  • 1
    \$\begingroup\$ @Johan new ValueChange<int>(1000, Blah.GetMemoryValue) (note the missing ()). \$\endgroup\$ – svick Nov 21 '13 at 20:05
  • 1
    \$\begingroup\$ You might also want to have a clean way of stopping the timer and tearing it down (i.e. stop watching) \$\endgroup\$ – ChrisWue Nov 21 '13 at 20:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.