6
\$\begingroup\$

I have a service that monitors failure and success directories for files that are generated by an engineering program. I want to check the change was in the success or failure folder, and if it was a new file creation. When those criteria are met I need to check if the file type matches one of the keys for a dictionary, and if it does I want to copy that file to a destination as declared in the config file.

I have an if statement that says:

if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.successFolder)
        && e.ChangeType == WatcherChangeTypes.Created)
{
    string fileExtension = Path.GetExtension(e.FullPath);
    try
    {
        //if the file type is desired copy to destinations.
        if( this.successDictionary.ContainsKey(fileExtension) )
        {
            CopyToDestination(this.successDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}
//if a file was added to the fail folder
else if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.failFolder)
            && e.ChangeType == WatcherChangeTypes.Created)
{
    activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
    string fileExtension = Path.GetExtension(e.FullPath);
    try
    {
        //if the file type is desired copy to destinations.
        if (this.failDictionary.ContainsKey(fileExtension))
        {
            CopyToDestination(this.failDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

Is it better to do it the first way or the second way?

string fileExtension = Path.GetExtension(e.FullPath);
if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.successFolder)
        && e.ChangeType == WatcherChangeTypes.Created)
{
    try
    {
        //if the file type is desired copy to destinations.
        if( this.successDictionary.ContainsKey(fileExtension) )
        {
            CopyToDestination(this.successDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}
//if a file was added to the fail folder
else if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.failFolder)
            && e.ChangeType == WatcherChangeTypes.Created)
{
    activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
    try
    {
        //if the file type is desired copy to destinations.
        if (this.failDictionary.ContainsKey(fileExtension))
        {
            CopyToDestination(this.failDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

It's really just a question of if it is better to sometimes waste a bit of memory, or if it is better to have it more optimized. I understand that in the grand scheme of things it's rather minimal how much memory it uses, but the voices in my head can't decide which one is better.

\$\endgroup\$
6
\$\begingroup\$

In this case, it would likely not make a difference. It is one call into the standard library that does a string operation that produces a short lived variable. If you want to know the correct answer, you should profile the code with real data to see what performs better.

If you don't care enough to profile it, do whatever lets you sleep better at night.


An optimization that you can do to speed up the code is change the if clauses.

  • In both cases, you execute NormalizePath(Path.GetDirectoryName(e.FullPath)).

    This can be done once, then use the result in the string comparison of the if clause.

  • Perform the check of e.ChangeType first.

    If the type doesn't match, you don't need do any path manipulation. Simply moving that check to the left side of the && instead of the right means the boolean expression can short circuit after a cheap operation before doing the expensive one. Also, since both cases are checking for the same type, you can pull the whole check out to an outer if block.

  • Do this.failFolder or this.successFolder change?

    If not, you can store the normalized path as a instance variable and not perform the operation each time.


The IOException is being handled in the same way and (it appears that) CopyToDestination() is the only thing that would cause it. You reduce this down to one try catch instead of two.


In general, you shouldn't be commenting your if clauses. If the boolean expression is complicated enough that it needs to be described, it should be extracted into a function with a descriptive name. In theses cases, the check being performed is straight forward.


Throw that all together and you get something like this:

if (e.ChangeType == WatcherChangeTypes.Created)
{
    var normalizedPath = NormalizePath(Path.GetDirectoryName(e.FullPath));
    try
    {
        if (normalizedPath == this.normalizeSuccessFolder)
        {
            string fileExtension = Path.GetExtension(e.FullPath);
            if(this.successDictionary.ContainsKey(fileExtension))
            {
                CopyToDestination(this.successDictionary[fileExtension], e);
            }
        }
        else if (normalizedPath == this.normalizeFailFolder)
        {
            activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
            string fileExtension = Path.GetExtension(e.FullPath);
            if (this.failDictionary.ContainsKey(fileExtension))
            {
                CopyToDestination(this.failDictionary[fileExtension], e);
            }
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

Edit:

Extracting common code into a method as pointed out by ChrisWue. There is still a repeated pattern there, but with just two cases, it isn't needed to abstract it more.

Had to make an assumption about the dictionary type and make up a name for the type of e. Even though this code doesn't care if the copy is attempted, I added a boolean return to indicate if it did. I also switched to using TryGetValue() instead of indexing into the dictionary twice.

private bool AttemptCopyToDestination(IDictionary<string,string> knownExtensions, SomeTypeName e)
{
    string fileExtension = Path.GetExtension(e.FullPath);
    string destination;
    if (knownExtensions.TryGetValue(fileExtension, out destination)
    {
        CopyToDestination(destination, e);
        return true;
    }
    return false;
}

// ...

if (e.ChangeType == WatcherChangeTypes.Created)
{
    var normalizedPath = NormalizePath(Path.GetDirectoryName(e.FullPath));
    try
    {
        if (normalizedPath == this.normalizeSuccessFolder)
        {
            AttemptCopyToDestination(this.successDictionary, e);
        }
        else if (normalizedPath == this.normalizeFailFolder)
        {
            activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
            AttemptCopyToDestination(this.failDictionary, e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Beautiful. Well worded, descriptive, and your code looks much nicer than mine does. I'm still pretty new to programming (second year of college and about 1 yr at a pt time internship). Your response has made me unreasonably happy. \$\endgroup\$ – PugsOverDrugs Jan 2 '15 at 18:58
  • 2
    \$\begingroup\$ You have duplicate code in the copying logic (getting the extension and then copying the file to destination if the extension is in a certain dictionary). This should be extracted into a common method. \$\endgroup\$ – ChrisWue Jan 3 '15 at 2:29

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.