debugcsharpMinor
Lower level exception handling during enumeration
Viewed 0 times
enumerationhandlingexceptionlowerlevelduring
Problem
I've got pretty straight forward scenario. I've got a class called
Exceptions I consider here that should be handled are
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
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
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:
The need of
Probably you can live with calling GetHashCode() of the file path which is of the
That only takes 1 line of code.
It seems that, in some way, the responsibility of the current class could be moved into
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
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
A better alternative in my perspective could be to support an event that calls the subscribers when an error occurs:
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 insteadof 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.