I don't know the actual reasons why it was done this way, and to a certrain degree I agree that preventing infintely wide exceptions from being thrown would be a good thing.
BUT... when coding small demo apps or proof-of-concepts, I don't want to start designing 10 different exception sub-classes, or spending time trying to decide which is the "best" exception class for the situation. I'd rather just throw Exception
and pass a string that explains the details. When it's throw-away code, I don't care about these things and if I was forced to care about such things, I'd either create my own GenericException
class and throw that everywhere, or move to a different tool/language. For some projects, I agree that properly creating relevant Exception subclasses is important, but not all projects require that.
You're doing quite a lot in a single class, that being said let's see what we can do.
First lets modify the code so that we can better support OCP. We will do this by introducing a dictionary that will contain your processors and a related interface IFileTypeProcessor.
interface IFileTypeProcessor
{
// not sure what string you intended to return
string Process(FileStream fileStream);
}
class FileProcessor : IFileProcessor
{
private readonly IDictionary<string, IFileTypeProcessor> processors;
public FileProcessor(IDictionary<string, IFileTypeProcessor> processors)
{
this.processors = processors
}
public string Process(string fileName)
{
using (FileStream fileStream = new FileStream(fileName, FileMode.Open, FileAccess.Read))
{
BinaryReader binaryReader = new BinaryReader(fileStream);
string hexString = BinaryFunctions.BytesToHexString(binaryReader.ReadBytes(2));
string result = "";
if(!this.processors.ContainsKey(hexString))
{
throw new YourApplicationSpecificException();
}
return this.processors[hexString].Process(fileStream);
}
}
}
Next let's introduce a new class called FirstTwoByteHexKeyProvider that implements a new interface IKeyREader which will then become an additional dependency on the CTOR. We'll also include some guard clauses to help our users because we like them.
interface IKeyReader
{
string GetKeyAsHexString(FileStream filestream)
}
class FirstTwoByteHexKeyProvider : IKeyReader
{
public string GetKeyAsHexString(FileStream fileStream)
{
if(fileStream)
{
throw new ArgumentNullException(nameof(fileStream));
}
BinaryReader binaryReader = new BinaryReader(fileStream);
return BinaryFunctions.BytesToHexString(binaryReader.ReadBytes(2));
}
}
class FileProcessor : IFileProcessor
{
private readonly IDictionary<string, IFileTypeProcessor> processors;
private readonly IKeyReader keyReader;
public FileProcessor(IDictionary<string, IFileTypeProcessor> processors, IKeyReader keyReader)
{
if(processors == null)
{
throw new ArgumentNullException(nameof(fileStream));
}
if(keyReader == null)
{
throw new ArgumentNullException(nameof(keyReader));
}
this.processors = processors
this.keyReader = keyReader
}
public string Process(string fileName)
{
using (FileStream fileStream = new FileStream(fileName, FileMode.Open, FileAccess.Read))
{
string hexString = this.keyReader.GetKeyAsHexString(fileStream);
if(!this.processors.ContainsKey(hexString))
{
throw new YourApplicationSpecificException();
}
return this.processors[hexString].Process(fileStream);
}
}
}
Now on to dependencies, do you need to support only Ninject? if so it could be as simple as using NinjectModules:
public class FileProcessorNinjectModule : NinjectModule
{
public override void Load()
{
Bind<IFileProcessor>().To<FileProcessor>();
Bind<IKeyReader>().To<FirstTwoByteHexKeyProvider>();
Bind<IDictionary<string, IFileTypeProcessor>>().ToMethod(context => new Dictionary<string,IFileTypeProcessor>
{
["123"] = new FileType1Processor(),
["456"] = new FileType2Processor(),
});
}
}
Once you have the module built tell your customers to use some form of Dynamic Module Loading and Bob's your uncle.
I wrote this quickly here and didn't test it via compiler but I'm sure it'll work with a bit of correction on your part but it should be enough to get you going.
If you have to support more than one form of DI then I'd recommend exposing an interface that will allow the customer to provide their own DI Container to you and your package will then use that to resolve dependencies much the same way that ASP.NET Core does it today.
Lastly you may consider ripping out the action of actually reading the file into another class and making the binary result of this "file read" an argument that get's passed in to your process method instead of the fileName since as it stands this will be a bit of a pain to test.
If you don't want to do that then you at the very least can extract the "new FileStream(..." into an internal virtual function which will give you the ability to MOQ around this system call in unit testing.
Hope that helps...
Best Answer
No, you shouldn't.
You should pass in connection strings as a dependency, or use
ConfigurationManager
to pick them up from the application configuration.Hard coding them means that you can't change them without recompiling the libraries.
False logic there. You should handle database issues in that data access layer (which can be in a class library) - other logical and business issues should be handled in the business logic layer (which can also be its own class library).
Exceptions that are not handled bubble up.