Lower level exception handling during enumeration

Question

I've got pretty straight forward scenario. I've got a class called FileInformationExtractor and method GetFilesInformation that takes directory path to find files within it and yields specific information about each of them. Exceptions I consider here that should be handled are IOException and UnauthorizedAccessException. Now I don't want to break enumeration so I've got try/catch block within the loop to process all the files. On the other hand I don't want to pretend that nothing happened and family's still happy so first I considered logging, but since this is quite low level class in my system I'd hate to reference log library from basicaly IO extension library. The solution I came up with is to first handle the exceptions, but instead of loosing them I collect them inside the List<Exception and throw as AggregateException after enumeration is complete. I'd like to know if you have any suggestions for alternative approaches, perhaps something I could improve about the code itself, good practices and so on.

/// <summary>
/// Provides information about files
/// </summary>
public class FileInformationExtractor : IFileInformationExtractor
{
    private readonly IFileFinder _fileFinder;
    private readonly IFileHelper _fileHelper;
    private readonly IFileHasher _hasher;

    /// <summary>
    /// Default constructor.
    /// </summary>
    /// <param name="fileFinder">Movie finder</param>
    /// <param name="fileHelper">File helper</param>
    /// <param name="hasher">File hasher</param>
    /// <exception cref="T:System.ArgumentNullException">Throws when any of constructor parameters is null.</exception>
    public FileInformationExtractor( IFileFinder fileFinder, IFileHelper fileHelper, IFileHasher hasher)
    {
        if ( fileFinder == null ) throw new ArgumentNullException( nameof( fileFinder ) );
        if ( fileHelper == null ) throw new ArgumentNullException( nameof( fileHelper ) );
        if ( hasher == null ) throw new ArgumentNullException( nameof( hasher ) );

        _fileFinder = fileFinder;
        _fileHelper = fileHelper;
        _hasher = hasher;
    }

    /// <summary>
    /// Searches given directory looking for movie files, generates their hash. 
    /// </summary>
    /// <param name="path">Directory</param>
    /// <returns>Collection of FileInformation</returns>
    /// <exception cref="T:System.AggregateException">Throws after enumeration is complete 
    /// if any exceptions were handled during it.</exception>
    public IEnumerable<FileInformation> GetFilesInformation( string path )
    {
        var files = _fileFinder.Find( path, true );
        var handledExceptins = new List<Exception>();

        foreach ( var filePath in files )
        {                
            var fileInformation = new FileInformation();
            try
            {
                var hash = _hasher.GenerateHash( filePath );
                fileInformation.Path = filePath;
                fileInformation.Hash = hash;
            }
            catch ( UnauthorizedAccessException exception )
            {
                handledExceptins.Add( exception );
            }
            catch ( IOException exception )
            {
                handledExceptins.Add( exception );
            }

            if ( IsInformationNotEmpty( fileInformation ) )
            {
                yield return fileInformation;
            }
        }

        if ( handledExceptins.Count > 0 )
        {
            throw new AggregateException(handledExceptins);
        }
    }

    private bool IsInformationNotEmpty( FileInformation fileInformation )
    {
        return !string.IsNullOrWhiteSpace( fileInformation.Hash );
    }
}

Apparently writing the problem down helped me out a little. I recalled TryGet... kind of methods, that handle exceptions and use out to get the result so I modified code a little introducing custom exception. I like that approach better than previous one. Should I worry about other exceptions? I'm also really looking forward for any naming suggestions (figuring out how to call all the stuff takes me ages :P).

/// <summary>
/// Provides information about files
/// </summary>
public class FileInformationExtractor : IFileInformationExtractor
{
    private readonly IFileFinder _fileFinder;
    private readonly IFileHelper _fileHelper;
    private readonly IFileHasher _hasher;

    /// <summary>
    /// Default constructor.
    /// </summary>
    /// <param name="fileFinder">Movie finder</param>
    /// <param name="fileHelper">File helper</param>
    /// <param name="hasher">File hasher</param>
    /// <exception cref="T:System.ArgumentNullException">Throws when any of constructor parameters is null.</exception>
    public FileInformationExtractor( IFileFinder fileFinder, IFileHelper fileHelper, IFileHasher hasher)
    {
        if ( fileFinder == null ) throw new ArgumentNullException( nameof( fileFinder ) );
        if ( fileHelper == null ) throw new ArgumentNullException( nameof( fileHelper ) );
        if ( hasher == null ) throw new ArgumentNullException( nameof( hasher ) );

        _fileFinder = fileFinder;
        _fileHelper = fileHelper;
        _hasher = hasher;
    }

    /// <summary>
    /// Searches given directory looking for movie files, generates their hash. 
    /// </summary>
    /// <param name="path">Directory</param>
    /// <returns>Collection of FileInformation</returns>
    /// <exception cref="T:Common.IO.FileHashingException">Throws after enumeration is complete 
    /// if any exceptions were handled during it.</exception>
    public IEnumerable<FileInformation> GetFilesInformation( string path )
    {
        var files = _fileFinder.Find( path, true );
        var notProcessedFiles = new List<string>();

        foreach ( var filePath in files )
        {
            FileInformation fileInformation;

            if ( TryGetFileInformation( filePath, out fileInformation ) )
            {
                yield return fileInformation;
            }
            else
            {
                notProcessedFiles.Add( filePath );
            }
        }

        if ( notProcessedFiles.Count > 0 )
        {
            throw new FileHashingException( notProcessedFiles );
        }
    }

    private bool TryGetFileInformation( string filePath, out FileInformation fileInformation )
    {
        try
        {
            var hash = _hasher.GenerateHash( filePath );

            fileInformation = new FileInformation
            {
                Path = filePath,
                Hash = hash
            };

            return true;
        }
        catch (IOException e)
        {
            fileInformation = null;
            return false;
        }
        catch (UnauthorizedAccessException e)
        {
            fileInformation = null;
            return false;
        }
    }
}

public class FileHashingException : Exception
{
    private const string ExceptionMessage = "There were exceptions when trying to hash listed files.";
    public IEnumerable<string> FilesCausingException { get; }

    public FileHashingException( IEnumerable<string> filesCausingException ) 
        : base(ExceptionMessage)
    {
        FilesCausingException = filesCausingException;
    }
}

Answer 1

I think you might be over-designing.

Let's start by taking a look to the class dependencies:

private readonly IFileFinder _fileFinder;
private readonly IFileHelper _fileHelper;
private readonly IFileHasher _hasher;

_fileHelper is not used anywhere and can be removed. The need of _hasher is questionable unless what you want to do here is to hash the contents from the file. Probably you can live with calling GetHashCode() of the file path which is of the string type, which has a really good hashing algorithm. That only takes 1 line of code.

It seems that, in some way, the responsibility of the current class could be moved into IFileFinder, so that interface could return FileInformation on the Find method instead of just returning strings. However I can live with that dependency if it makes some sense somehow, so I leave that decision up to you.

If I take a look closely to GetFilesInformation I can't see why it should throw any exception at all. All you are doing there is creating a hash and putting the file path on a property. So again, unless the _hasher hashes the contents of a file there shouldn't be any exception.

One thing that you can do is to create a variable and check if it was assigned or not, yes you have to make sure that you call the methods that can fail first but well, I guess it's quite understandable and a easy solution in this scenario. You can also extract the code to another method. The implementation it's quite similar to the one provided by @Heslacher, except it returns null on exception. I prefer that way because I hate to write methods with out and ref keywords (I always will, it's my preference it might be even be a narrow minded one, but that does not bother me).

A better alternative in my perspective could be to support an event that calls the subscribers when an error occurs:

public class FileErrorArgs : EventArgs{
    public IEnumerable<string> Files{ get; set; }
}
public event EventHandler<FileErrorArgs> FileError;

public IEnumerable<FileInformation> GetFilesInformation( string path )
{
    var files = _fileFinder.Find( path, true );
    var invalidFiles = new List<string>();

    foreach ( var filePath in files )
    {                
        FileInformation fileInformation = GetFileInformation(string filePath);
        if(fileInformation == null){
            invalidFiles.Add(filePath);
        }else{
            yield return fileInformation;
        }
    }

    if ( invalidFiles.Count > 0 )
    {
        var handler = FileError;
        if(handler != null){
            handler.Invoke(new FileErrorArgs{
                Files = invalidFiles
            });
        }
    }
}

public FileInformation GetFileInformation(string filePath){
    try
    {
        return new FileInformation(){
            Hash = _hasher.GenerateHash(filePath),
            Path = filePath
        };
    }
    catch ( UnauthorizedAccessException exception )
    {
        return null;
    }
    catch ( IOException exception )
    {
        return null;
    }
}

Answer 2

GetFilesInformation()

If there is a chance that _fileFinder.Find() throws an exception for the case that the passed in path is either null, string.Empty, non existent or invalid then you should add a private method for doing the yielding and let either GetFilesInformation method take care about the validation of the passed in path. This is because the method in its current start will be on hold until an enumerator is created and the first item is queried.

Something along these lines

public IEnumerable<FileInformation> GetFilesInformation(string path)
{
    if (path == null) { throw new ArgumentNullException("path"); }
    if (string.IsNullOrWhiteSpace(path) { throw new ArgumentException("Path may not be empty or whitespace); }  
    if (!System.IO.Directory.Exists(path)) { throw new DirectoryNotFoundException("The path " + path + " could not be found"); }

    return GetFilesInformationEx(path);
}

private IEnumerable<FileInformation> GetFilesInformationEx(string path)
    var files = _fileFinder.Find(path, true);
    var notProcessedFiles = new List<string>();

    foreach (var filePath in files)
    {
        FileInformation fileInformation;

        if (TryGetFileInformation(filePath, out fileInformation))
        {
            yield return fileInformation;
        }
        else
        {
            notProcessedFiles.Add(filePath);
        }
    }

    if (notProcessedFiles.Count > 0)
    {
        throw new FileHashingException(notProcessedFiles);
    }
}

TryGetFileInformation()

If you set fileInformation to null at the very top, you wouldn't need to repeat it twice (in the catch statements).

like so

private bool TryGetFileInformation(string filePath, out FileInformation fileInformation)
{
    fileInformation = null;

    try
    {
        var hash = _hasher.GenerateHash(filePath);

        fileInformation = new FileInformation
        {
            Path = filePath,
            Hash = hash
        };

        return true;
    }
    catch (IOException e)
    {
        return false;
    }
    catch (UnauthorizedAccessException e)
    {
        return false;
    }
}

Answer 3

Thanks to both of you @Heslacher and @Bruno Costa I've come up with solution I'm quite satisfied with.

First the directory path validation. I decided to keep it within IFileFinder implementation so FileInformationExtractor can be separated from System.IO namespace. When finder will throw, regardless if it's invalid path, argument or none existing directory this class wont handle it, so thats right.

Next thing to achieve full independency was to handle both IOException and UnauthorizedAccessException within IFileHasher implementation, where any of them will be packed inside FileHashingException which FileInformationExtractor can smoothly handle with single catch block. Also @Bruno Costa, I share that distaste with out and ref so I took the null check approach. Exceptions that will be handled are now available through the event. I considered propagating just the message or the path, but having full exception will give me some more flexibility when designing higher level of the application ().

/// <summary>
/// Provides information about files
/// </summary>
public class FileInformationExtractor : IFileInformationExtractor
{
    private readonly IFileFinder _fileFinder;
    private readonly IFileHasher _hasher;

    /// <summary>
    /// Notifies about exceptions during file hashing.
    /// </summary>
    public event EventHandler<FileHashingException> FileHashingFailure;

    /// <summary>
    /// Default constructor.
    /// </summary>
    /// <param name="fileFinder">Movie finder</param>
    /// <param name="hasher">File hasher</param>
    /// <exception cref="T:System.ArgumentNullException">Throws when any of constructor parameters is null.</exception>
    public FileInformationExtractor( IFileFinder fileFinder, IFileHasher hasher)
    {
        if ( fileFinder == null ) throw new ArgumentNullException( nameof( fileFinder ) );
        if ( hasher == null ) throw new ArgumentNullException( nameof( hasher ) );

        _fileFinder = fileFinder;
        _hasher = hasher;
    }

    /// <summary>
    /// Searches given directory looking for movie files, generates their hash. 
    /// </summary>
    /// <param name="path">Directory</param>
    /// <returns>Collection of FileInformation</returns>
    public IEnumerable<FileInformation> GetFilesInformation( string path )
    {
        var files = _fileFinder.Find( path, true );

        foreach (var filePath in files)
        {
            var fileInformation = GetFileInformation( filePath );

            if (fileInformation != null)
            {
                yield return fileInformation;
            }
        }
    }

    private FileInformation GetFileInformation( string filePath )
    {
        try
        {
            var hash = _hasher.GenerateHash( filePath );

            return new FileInformation
            {
                Path = filePath,
                Hash = hash
            };
        }
        catch (FileHashingException e)
        {
            OnFileHashingFailure( e );
            return null;
        }
    }

    protected virtual void OnFileHashingFailure( FileHashingException e )
    {
        FileHashingFailure?.Invoke( this, e );
    }
}

/// <summary>
/// Represents error that occured during while file hashing operation.
/// </summary>
public class FileHashingException : Exception
{
    /// <summary>
    /// Path to the file that caused the exception
    /// </summary>
    public string FilePath { get; }

    /// <summary>
    /// Specific constructor.
    /// </summary>
    /// <param name="filePath">Path</param>
    /// <param name="message">Message</param>
    /// <param name="innerException">Inner exception</param>
    public FileHashingException( string filePath, string message, Exception innerException ) 
        : base(message, innerException)
    {
        FilePath = filePath;
    }
}