1
\$\begingroup\$

I wrote a little function that monitors some directories. If a file gets created, it will check in the extension list for a match. Can you please tell me if I could make my function better in any way?

private void sysWatcher_Created(object sender, FileSystemEventArgs e) {

    try {

        if (File.Exists(e.FullPath)) {

            var fileName = Path.GetFileName(e.FullPath);
            var fullPath = e.FullPath;
            var changeType = e.ChangeType;
            var fileExtension = Path.GetExtension(e.FullPath);

            _extensionList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));
            _ignoreList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));

            // Checking for match in Extension List
            bool _extResult = _extensionList.Any(s => e.FullPath.Contains(s));

            if (_extResult) {

                // Checking for match in Ignore List
                bool _ignResult = _ignoreList.Any(s => e.FullPath.Contains(s));

                if (!_ignResult) {

                    if (!GeneralSettings.autoMode) { 

                        App.Current.Dispatcher.Invoke(delegate {

                            if (File.Exists(e.FullPath)) {
                                var fileInfo = new FileInfo(e.FullPath);
                                var wFunc = new WarningItem(e.FullPath, e.Name, AppSettings.SizeSuffix(fileInfo.Length),
                                    fileInfo.CreationTime.ToString("g"), fileInfo.Extension);

                                var wWin = new WarningWin();
                                wWin.init(wFunc);
                                wWin.ShowDialog();

                                // Add to Warning List and List Box 
                                _warningList.Add(new WarningItem {
                                    DirName = Path.GetDirectoryName(e.FullPath),
                                    CreationDate = DateTime.Now.ToString("g"),
                                    Extension = fileInfo.Extension,
                                    FullPath = e.FullPath,
                                    FileName = e.Name,
                                    FileSize = AppSettings.SizeSuffix(fileInfo.Length),
                                    StatusImage = new Uri("/Images/inWatch.Folder.png", UriKind.RelativeOrAbsolute)
                                });
                                lstWarning.ItemsSource = _warningList;
                                tabWarning.IsSelected = true;
                                cmdSearchWarnings.Visibility = Visibility.Visible;
                            }
                        });
                    }
                    else {

                        Scan.scanFile(e.FullPath);
                    } 
                }
            }
        }            
    }
    catch (Exception ex) {
        GeneralSettings.LogException(ex);
    }
}
\$\endgroup\$
1
\$\begingroup\$

You have a good example of the arrow antipattern here but this can be easily resolved by inverting the conditions and using return.

if (!File.Exists(e.FullPath)) { return; }

_extensionList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));
_ignoreList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));

// Checking for match in Extension List
bool _extResult = _extensionList.Any(s => e.FullPath.Contains(s));

if (!_extensionList.Any(s => e.FullPath.Contains(s))) { return ; }

if (_ignoreList.Any(s => e.FullPath.Contains(s))) { return; }

if (GeneralSettings.autoMode) 
{
     Scan.scanFile(e.FullPath);
     return;
}
App.Current.Dispatcher.Invoke(delegate {

                        if (File.Exists(e.FullPath)) {
                            var fileInfo = new FileInfo(e.FullPath);
                            var wFunc = new WarningItem(e.FullPath, e.Name, AppSettings.SizeSuffix(fileInfo.Length),
                                fileInfo.CreationTime.ToString("g"), fileInfo.Extension);
....
.... 
....}

In this way your code becomes more readable because of the reduced horizontal spacing.


About @janos point about the `Pointless Variables":

These variables are not only pointless but they are also never used.

\$\endgroup\$
  • \$\begingroup\$ Thanks such a simple Solution. Never seen it like that , now the Code looks much cleaner and easier to Read. Yeah those "Pointless Variables" where never used :) i removed it. \$\endgroup\$ – iNCEPTiON_ Jul 6 '15 at 8:03
4
\$\begingroup\$

Several odd elements in this code.

Pointless variables

These variables look pointless:

        var fullPath = e.FullPath;
        var changeType = e.ChangeType;

You could use e.FullPath and e.ChangeType where you need it, the extra local variables don't seem to have any purpose.

Garbage not filtered out soon enough

It's very strange to execute this code every time a file is created:

        _extensionList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));
        _ignoreList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));

Why do you need to filter out the garbage every time instead of doing it just once?

Even better: how did garbage every enter these lists? It would be better to prevent that, and organize your code in a way that it's impossible to enter garbage into these lists.

Lastly, the result of r.Trim() will never be null, so the null-check is pointless.

Why not use a Set ?

Instead of putting the extensions and ignored extensions into lists, it would be better to use sets instead. It will it a lot more efficient to check if they contain an element or not, without the need for lambdas.

Exception handling

The try block is to large. It's better to limit the scope of try blocks, to make it easier to see the piece of code where an operation might fail.

Instead of catching a generic Exception, it's better to catch the most specific kind of exception that can be thrown.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer! Wow i didnt see that those variables are useless, better 4 eyes than 2 :) I have the extension and Ignore elements saved to a Textfile that i read at application start but everytime i read the textfiles there are empty entrys in the List. What do you mean by Set's ? I had a strange feeling about the Exception block, should catch the specific. \$\endgroup\$ – iNCEPTiON_ Jul 4 '15 at 13:30
  • \$\begingroup\$ It's better to clean garbage as early as possibly. It's better to never add empty strings in the list of extensions in the first place, so you don't have to filter them later. A set of a data structure with unique elements, and typically allowing a fast way to check if it contains an element or not \$\endgroup\$ – Stop ongoing harm to Monica Jul 4 '15 at 16:03
  • \$\begingroup\$ Check out msdn.microsoft.com/en-us/library/bb359438(v=vs.110).aspx \$\endgroup\$ – Heslacher Jul 4 '15 at 22:10

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.