HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

Lower level exception handling during enumeration

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
enumerationhandlingexceptionlowerlevelduring

Problem

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.

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

///
/// Default constructor.
///
/// Movie finder
/// File helper
/// File hasher
/// Throws when any of constructor parameters is null.
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;
}

///
/// Searches given directory looking for movie

Solution

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 Files{ get; set; }
}
public event EventHandler FileError;

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

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

Code Snippets

private readonly IFileFinder _fileFinder;
private readonly IFileHelper _fileHelper;
private readonly IFileHasher _hasher;
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;
    }
}

Context

StackExchange Code Review Q#141994, answer score: 5

Revisions (0)

No revisions yet.