4
\$\begingroup\$

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);
    }


}
\$\endgroup\$

1 Answer 1

Reset to default
3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$ Commented May 5, 2014 at 9:32
  • \$\begingroup\$ @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)? \$\endgroup\$ Commented May 5, 2014 at 11:33
  • \$\begingroup\$ I wasn't aware of Nullable<T> \$\endgroup\$ Commented May 5, 2014 at 12:06
  • \$\begingroup\$ 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. \$\endgroup\$ Commented May 7, 2014 at 14:49
  • \$\begingroup\$ @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! \$\endgroup\$ Commented May 7, 2014 at 14:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.