IFileSystem Dependency Inversion Part 5
The saga began here.
Where I left off, I'd managed to create a new class for handling the storage of my creative files, called CreativeFileStore. This method took in an IFileSystem as a parameter to its constructor, which provides two benefits:
- Testability
- Flexibility - I can swap between WindowsFileSystem and AmazonS3FileSystem easily
In the interest of keeping the individual posts at a reasonable size, I didn't include all the tests for the CreativeFileStore, but here's a summary:
1: [TestMethod]2: public void Create_CreativeFileStore(){}
3: 4: [TestMethod]5: [ExpectedException(typeof (IOException))]
6: public void WriteFile_Fails_If_File_Already_Exists(){}
7: 8: [TestMethod]9: public void WriteFile_Calls_WriteBinaryFile_When_No_Dimensions_Passed(){}
10: [TestMethod]11: public void WriteFile_Calls_WriteBinaryFile_When_Dimensions_Passed_But_File_Is_Not_Image()
12: {} 13: 14: [TestMethod]15: [ExpectedException(typeof (ApplicationException))]
16: public void WriteFile_Throws_Exception_When_Incorrect_Dimensions_Passed(){}
17: 18: [TestMethod]19: public void WriteFile_Calls_WriteBinaryFile_When_Dimensions_Passed_And_Image_Is_Correct_Size()
20: {}In the last post, when I finished up there was a nasty call to FormatAndSaveUploadedImage() that was failing. I wrapped up by commenting it out and just saving the image (without validating its size), which was obviously not a long term solution. That method call goes to a fairly old and procedural library of static calls, and in this case it's trying to do way too much and is completely violating SRP. I decided to fix it - the original method isn't worth describing here.
I decided that validating the height and width of an image file was worthy of its own class, so I created a simple ImageValidator class that implements its very own IImageValidator interface (you'll see why in a moment). The interface looks like this:
1: public interface IImageValidator
2: {3: int ExpectedHeight { get; set; }
4: int ExpectedWidth { get; set; }
5: System.Drawing.Image ImageToValidate { get; set; } 6: 7: void SetImageFromBytes(byte[] imageToValidate);
8: bool IsExpectedHeight();
9: bool IsExpectedWidth();
10: bool IsExpectedDimensions();
11: }The only interesting code from the ImageValidator is here:
1: public bool IsExpectedHeight()
2: {3: if(ImageToValidate == null)
4: {5: throw new ArgumentNullException("ImageToValidate",
6: "ImageToValidate property must be specified.");
7: }8: float imageHeight = ImageToValidate.PhysicalDimension.Height;
9: if (imageHeight != ExpectedHeight) return false;
10: 11: return true;
12: } 13: 14: public bool IsExpectedWidth()
15: {16: if (ImageToValidate == null)
17: {18: throw new ArgumentNullException("ImageToValidate",
19: "ImageToValidate property must be specified.");
20: }21: float imageWidth = ImageToValidate.PhysicalDimension.Width;
22: if (imageWidth != ExpectedWidth) return false;
23: 24: return true;
25: } 26: 27: public bool IsExpectedDimensions()
28: {29: return IsExpectedHeight() && IsExpectedWidth();
30: }Now the WriteFile() method can simply do the validation itself, rather than rely on it happening within a rather long and cumbersome (and untestable) static method:
1: _imageValidator.SetImageFromBytes(creativeFile.Bytes); 2: _imageValidator.ExpectedHeight = expectedHeightWidth.FirstValue; 3: _imageValidator.ExpectedWidth = expectedHeightWidth.SecondValue; 4: 5: if (!_imageValidator.IsExpectedDimensions())
6: {...} 7: Note that to make this work, we need to inject the dependency on IImageValidator via the constructor (or the method). We were already doing this with the IFileSystem - the complete code for the CreativeFileStore is shown here:
1: public class CreativeFileStore
2: {3: private readonly IFileSystem _fileSystem;
4: private readonly IImageValidator _imageValidator;
5: 6: public CreativeFileStore() :
7: this(new WindowsFileSystem(), new ImageValidator()) { }
8: 9: public CreativeFileStore(IFileSystem fileSystem,
10: IImageValidator imageValidator) 11: {12: this._fileSystem = fileSystem;
13: this._imageValidator = imageValidator;
14: } 15: 16: public void WriteFile(CreativeFile creativeFile,
17: Tuple<int, int> expectedHeightWidth, DateTime time)
18: {19: string fileName = creativeFile.GenerateStandardFileName(time);
20: 21: if (_fileSystem.FileExists(fileName))
22: {23: throw new IOException(
24: @"An error occurred which did not allow the uploading of your file. " +
25: @"Please try again or contact your account manager.");
26: } 27: 28: // if it's an image and we have expected dimensions,
29: //validate it and throw if it is invalid
30: if (CreativeType.GetTypeFromExtension(
31: creativeFile.GetFileExtension()) == CreativeTypes.Image &&32: expectedHeightWidth != null)
33: { 34: _imageValidator.SetImageFromBytes(creativeFile.Bytes); 35: _imageValidator.ExpectedHeight = expectedHeightWidth.FirstValue; 36: _imageValidator.ExpectedWidth = expectedHeightWidth.SecondValue; 37: 38: if (!_imageValidator.IsExpectedDimensions())
39: {40: throw new ApplicationException(
41: string.Format(
42: @"There was a problem uploading your image. Please verify that the width
43: height of the image you're uploading are exactly {0} x {1} pixels.", 44: expectedHeightWidth.SecondValue, expectedHeightWidth.FirstValue)); 45: } 46: } 47: _fileSystem.WriteBinaryFile(creativeFile.Bytes, fileName); 48: } 49: }Since we're using interfaces and DI, we're not only able to swap out our file system (or validation logic), we're also able to easily test scenarios that would otherwise involve complex setup. For instance, as I was wrapping up this exercise I thought it would be good to have a test showing what should happen when the image is of the expected height and width. In that case, the file should be written. Using the original static method, the only way I could get it to write the file would be to ensure that the actual byte array I passed it was a valid image file of the expected size. By refactoring to introduce an IImageValidator, I was able to test this case using the following code (which uses Rhino Mocks):
1: [TestMethod]2: public void WriteFile_Calls_WriteBinaryFile_When_Dimensions_Passed_And_Image_Is_Correct_Size()
3: {4: Expect.Call(myFileSystem.FileExists(CreativeFileTester.TEST_FILE_NAME)).IgnoreArguments().Return(false);
5: Expect.Call(myImageValidator.IsExpectedDimensions()).IgnoreArguments().Return(true);
6: Expect.Call(delegate { myFileSystem.WriteBinaryFile(CreativeFileTester.TEST_BYTES, ""); }).IgnoreArguments();
7: 8: mocks.ReplayAll();9: var myFileStore = new CreativeFileStore(myFileSystem, myImageValidator);
10: CreativeFile myCreativeFile = CreativeFileTester.GetTestCreativeFile();11: var expectedHeightWidth = new Tuple<int, int>(10, 10);
12: 13: myFileStore.WriteFile(myCreativeFile, expectedHeightWidth, CreativeFileTester.TEST_DATE); 14: mocks.VerifyAll(); 15: }Return To The Ugly Method
In the last part of this series, the GetImageOrFlashData() method was down to 20 lines. I decided to move the folder creation code to it, and now there's a little bit of setup code to create the necessary objects to do the work, but it's still much smaller than it was, and much easier to follow. The name isn't great, though, so I think one last refactoring before we call it a day is to rename it to SaveCreativeFile, since that's really what it's doing.
SaveCreativeFile:
1: private void SaveCreativeFile()
2: {3: var myCreativeFile = new CreativeFile
4: { 5: FileName = ImageOrFlashUpload.FileName, 6: Bytes = ImageOrFlashUpload.FileBytes 7: }; 8: 9: var myFileSystem = new WindowsFileSystem();
10: 11: var myCreativeFileStore = new CreativeFileStore(myFileSystem,
12: new ImageValidator());
13: try
14: {15: // Ensure Destination Folder Exists
16: string directoryLocation =
17: LQPromotionServerConfig.Settings.TemporaryCreativeBaseFilePath;18: if (!myFileSystem.DirectoryExists(directoryLocation))
19: { 20: myFileSystem.CreateDirectory(directoryLocation); 21: } 22: myFileSystem.StorageFolderPath = directoryLocation; 23: 24: var expectedHeightWidth = new Tuple<int, int>(
25: MyCreativeFormat.Height, MyCreativeFormat.Width); 26: myCreativeFileStore.WriteFile(myCreativeFile, 27: expectedHeightWidth, DateTime.Now); 28: }29: catch (Exception ex)
30: { 31: FileErrorLabel.Text = ex.Message;32: return;
33: } 34: }I added a few line breaks to make it fit the blog better - it's about 30 lines in Visual Studio. Definitely fits the rule that no method should be longer than can be viewed on the screen at one time (and I'm only on a 1050 line vertical resolution monitor at the moment). Having longer methods makes it much more difficult to grok what's going on, and is almost always a sign that the method is trying to do too much.
In fact, just to add one *last* refactoring to this, let me point out another sign -- comments. Often comments are a sign that what you really want to do is refactor, often with an Extract Method. The only comment I have left in the above code says "Ensure Destination Folder Exists". So... why do I need that when I could extract to a method called EnsureFolderExists(foldername)?
Something like this:
1: private void EnsureFolderExists(IFileSystem myFileSystem, string directoryLocation)
2: {3: if (!myFileSystem.DirectoryExists(directoryLocation))
4: { 5: myFileSystem.CreateDirectory(directoryLocation); 6: } 7: }This adds 7 lines to my total LOC for the file, but takes away 3 lines and, more importantly, the need for a comment, in the other method.
Now to write my AmazonS3FileSystem and see if we really can just flip a switch and use another service to host our files...
Comments
Robz said on 12 Dec 2008 at 9:21 AM
This is great! Great to see someone using DI and orthogonal development.
Busby Test said on 08 Jan 2009 at 12:04 PM
wow you great steve to make this work. thanks.
cisco telepresence said on 19 Sep 2009 at 6:56 AM
Steve are you releasing the code to public under GPL? We'd like to adapt the code but not sure about the licensing.
lawn fertilizer said on 24 Sep 2009 at 6:47 AM
This is why i love object orient code... simple, debug friendly and less code maintenance... keep it up Steve...