FileSystemWatcher firing multiple events

Question

Like others (FileSystemWatcher Changed event is raised twice) I am facing the problem that events of filesystemwatcher are raised twice. I require to catch all non-duplicate events of watcher in real time. I came up with this. So, I want to know if it will be efficient/over-kill/buggy to use this class.

class ModifiedFileSystemWatcher:FileSystemWatcher
{
 public new event RenamedEventHandler Renamed;
 public new event FileSystemEventHandler Deleted;
 public new event FileSystemEventHandler Created;
 public new event FileSystemEventHandler Changed;
 class BooleanWrapper{
 public bool value { get; set; }
 }
 //stores refrences of previously fired events with same file source
 Dictionary<string, BooleanWrapper> raiseEvent = new Dictionary<string, BooleanWrapper>();
 public int WaitingTime { get; set; } //waiting time, any new event raised with same file source will discard previous one
 public ModifiedFileSystemWatcher(string filename="", string filter="*.*"):base(filename,filter)
 {
 base.Changed += ModifiedFileSystemWatcher_Changed;
 base.Created += ModifiedFileSystemWatcher_Created;
 base.Deleted += ModifiedFileSystemWatcher_Deleted;
 base.Renamed += ModifiedFileSystemWatcher_Renamed;
 WaitingTime = 100;
 }
 void PreventDuplicate(FileSystemEventHandler _event, FileSystemEventArgs e)
 {
 if (_event != null)
 {
 //create a reference
 BooleanWrapper w = new BooleanWrapper() { value = true }; //this event will be fired
 BooleanWrapper t; //tmp
 if (raiseEvent.TryGetValue(e.FullPath, out t))
 {
 //a previous event occurred which is waiting [delayed by WaitingTime]
 t.value = false; //set its reference to false; that event will be skipped
 t = w;//store current reference in dictionary
 }
 else raiseEvent[e.FullPath] = w; //no previous event, store our reference
 //Wait on a separate thread...
 System.Threading.ThreadPool.QueueUserWorkItem(delegate
 {
 System.Threading.Thread.Sleep(WaitingTime);
 if (w.value) //if we are still allowed to raise event
 {
 _event(this, e);
 raiseEvent.Remove(e.FullPath);//remove instance from dictionary
 }
 }, null);
 }
 }
 //Same as above with different event
 void ModifiedFileSystemWatcher_Renamed(object sender, RenamedEventArgs e)
 {
 if (Renamed != null)
 {
 BooleanWrapper w = new BooleanWrapper() { value = true };
 BooleanWrapper t;
 if (raiseEvent.TryGetValue(e.FullPath, out t))
 {
 t.value = false;
 t = w;
 }
 else raiseEvent[e.FullPath] = w;
 System.Threading.Thread.Sleep(WaitingTime);
 if (w.value)
 {
 Renamed(this, e);
 raiseEvent.Remove(e.FullPath);
 }
 }
 }
 void ModifiedFileSystemWatcher_Deleted(object sender, FileSystemEventArgs e)
 {
 PreventDuplicate(Deleted, e);
 }
 void ModifiedFileSystemWatcher_Created(object sender, FileSystemEventArgs e)
 {
 PreventDuplicate(Created, e);
 }
 void ModifiedFileSystemWatcher_Changed(object sender, FileSystemEventArgs e)
 {
 PreventDuplicate(Changed, e);
 }
}

Answer 1

I copied your code into a new console application. Your constructor is "eating" the base default constructor, and now I can't pass a path parameter, I have to trust that filename will work if I give it a folder path. Let's see..

public ModifiedFileSystemWatcher(string filename="", string filter="*.*"):base(filename,filter)
{
 /* constructor */
}

This would be easier to read on two lines:

public ModifiedFileSystemWatcher(string filename="", string filter="*.*")
 : base(filename,filter)
{
 /* constructor */
}

Changing the names of constructor arguments is confusing. In this case you're passing filename as the base constructor's path parameter, but filter is passed as filter. I would rename filename to path to avoid confusion.

The optional parameters are surprising, too - a FileSystemWatcher exposes a parameterless constructor, one with only a path, and one with both a path and a filter. Your optional parameters add a constructor for a filter without a path.

---

This is a rather unusual way of assigning a value in a dictionary:

BooleanWrapper w = new BooleanWrapper() { value = true };
BooleanWrapper t;
if (raiseEvent.TryGetValue(e.FullPath, out t))
{
 t.value = false;
 t = w; //store current reference in dictionary
}

This might work, but I find raiseEvent[e.FullPath] = w; makes the intent much more explicit.

• Akshay Vats: Hi, Thanks for your review. I will change the constructor parameter names to make sense. For the second part, t=w instead of raiseEvent[e.FullPath] = w;. Yes that will increase readability. But my method skips searching dictionary again.
• Mathieu Guindon: @AkshayVats the "search" is O(n), which will be instant in most cases since you keep the dictionary clean. My rule of thumb is to favor readability over performance, unless there's an actual performance issue, in which case a profiler will identify where the bottleneck is. If you refactored BooleanWrapper usages to work with a Nullable<bool> instead, the immutability of the type would blow it up. What else do you need BooleanWrapper for (that a Nullable<bool> can't do)?
• Akshay Vats: I wasn't aware of Nullable<T>
• Akshay Vats: Hi, today I thought of removing BooleanWrapper. I learned about Nullable types, but they are struct. My point of creating wrapper was to have a reference. Give me some ideas.
• Mathieu Guindon: @AkshayVats I don't think it's efficient to have a reference in this case - if all you need is a flag that tells you whether or not you can/should raise the event, then I think the most readable and efficient way of doing it is to have raiseEvent be a Dictionary<string, bool>. Keep it simple!