IFileSystem Dependency Inversion Part 4
Still working on cleaning up some legacy ASP.NET code. Here's where we are:
- Part 1: Define problem and demonstrate IFileSystem basic version
- Part 2: Spike solution to support saving files in IFileSystem that works in both Amazon S3 and the Windows file system
- Part 3: Initial refactoring via TDD of big ugly method
Now it's time to take the big step of pulling the main ugly method guts out into its own object. Since the main purpose of the method, GetImageOrFlashData(), is to store a file that has been uploaded, I'm thinking at the moment that the name for the class who will take on this responsibility is going to be CreativeFileStore (a creative is a noun in this context; a creative file is a file representing a creative, not a file that uses its left brain). What should the CreativeFileStore's interface look like?
- Accepts an IFileSystem in its constructor
- Implements a WriteFile() method with required parameters for the existing method to use it to replace its current functionality.
Since we know our current implementation relies on the System.IO, it's simple enough to have the class use the WindowsFileSystem by default, resulting in a basic structure like so:
1: public class CreativeFileStore
2: {3: private readonly IFileSystem _fileSystem;
4: 5: public CreativeFileStore() : this(new WindowsFileSystem()) {}
6: 7: public CreativeFileStore(IFileSystem fileSystem)
8: {9: this._fileSystem = fileSystem;
10: } 11: }We also need to update IFileSystem (and its implementation) to support writing files based on the code we came up with during our spike solution (in part 2). Here's the new IFileSystem interface:
1: public interface IFileSystem
2: {3: bool FileExists(string path);
4: bool DirectoryExists(string path);
5: void CreateDirectory(string path);
6: void MoveFile(string oldPath, string newPath);
7: string ReadAllText(string path);
8: void WriteBinaryFile(byte[] bytes, string savingPath);
9: }And here's the WindowsFileSystem implementation of WriteBinaryFile:
1: public string StorageFolderPath { get; set; }
2: 3: public WindowsFileSystem()
4: { 5: StorageFolderPath = String.Empty; 6: } 7: 8: public void WriteBinaryFile(byte[] bytes, string savingPath)
9: {10: using (var writingStream =
11: new FileStream(
12: Path.Combine(StorageFolderPath, savingPath), 13: FileMode.OpenOrCreate) 14: ) 15: { 16: writingStream.Write(bytes, 0, bytes.Length); 17: writingStream.Flush(); 18: writingStream.Close(); 19: } 20: } 21:
We determined in the spike solution that the windows file system would need to know an upload path, so we've added a property StorageFolderPath to represent this. It will be up to whatever part of the system creates my implementation of IFileSystem to ensure that any additional properties like this one are properly set. In any event, if a full path is given to WriteBinaryFile, the lack of a StorageFolderPath shouldn't cause it to fail. That sounds like an assumption - let's verify with a test, shall we?
Pull out the Path.Combine into a separate method (extract method) which in our case we'll make protected since nothing outside this class should need it.
WindowsFileSystem (refactored):
1: public void WriteBinaryFile(byte[] bytes, string savingPath)
2: {3: using (var writingStream =
4: new FileStream(
5: GetFilePath(savingPath), 6: FileMode.OpenOrCreate) 7: ) 8: { 9: writingStream.Write(bytes, 0, bytes.Length); 10: writingStream.Flush(); 11: writingStream.Close(); 12: } 13: } 14: 15: protected string GetFilePath(string savingPath)
16: {17: return Path.Combine(StorageFolderPath, savingPath);
18: }Then we can test it easily enough by simply subclassing, like so:
TestWindowsFileSystem
1: public class TestWindowsFileSystem : WindowsFileSystem
2: {3: public string TestGetFilePath(string filePath)
4: {5: return GetFilePath(filePath);
6: } 7: }
Tests
1: [TestMethod]2: public void GetFilePath_Returns_Combined_Folder_And_File_Path()
3: {4: string folderPath = @"C:\Foo";
5: string filePath = "bar.txt";
6: string expectedFilePath = @"C:\Foo\bar.txt";
7: 8: TestWindowsFileSystem myFileSystem = new TestWindowsFileSystem();
9: myFileSystem.StorageFolderPath = folderPath; 10: Assert.AreEqual(expectedFilePath, myFileSystem.TestGetFilePath(filePath));11: myFileSystem.StorageFolderPath = folderPath + @"\";
12: Assert.AreEqual(expectedFilePath, myFileSystem.TestGetFilePath(filePath)); 13: } 14: 15: [TestMethod] 16: public void GetFilePath_Returns_File_Path_When_No_Folder_Set() 17: {18: string filePath = @"C:\Foo\bar.txt";
19: string expectedFilePath = @"C:\Foo\bar.txt"; 20: 21: TestWindowsFileSystem myFileSystem = new TestWindowsFileSystem();
22: Assert.AreEqual(expectedFilePath, myFileSystem.TestGetFilePath(filePath)); 23: }
These pass, so it sounds like we got this right. Now back to CreativeFileStore, which needs a method for writing files. This method replaces the goo that was in GetImageOrFlashData(), which when we're done should look pretty much like this:
1: private void GetImageOrFlashData()
2: {3: var myCreativeFile = new CreativeFile
4: { 5: FileName = ImageOrFlashUpload.FileName, 6: Bytes = ImageOrFlashUpload.FileBytes 7: }; 8: 9: var myCreativeFileStore = new CreativeFileStore();
10: try
11: { 12: myCreativeFileStore.WriteFile(myCreativeFile, 13: MyCreativeFormat, DateTime.Now); 14: }15: catch (Exception ex)
16: { 17: FileErrorLabel.Text = ex.Message;18: return;
19: } 20: }In the original implementation, we called into an upload control's PostedFile property to do SaveAs(filePath). We can now replace this with a call to our file system's WriteBinaryFile() method. The resulting method looks something like this (doesn't quite compile):
CreativeFileStore.WriteFile
1: public void WriteFile(CreativeFile creativeFile, CreativeFormat expectedFormat, DateTime time)
2: {3: string fileName = creativeFile.GenerateStandardFileName(time);
4: string directoryPath = LQPromotionServerConfig.Settings.TemporaryCreativeBaseFilePath;
5: string filePath = directoryPath + fileName;
6: 7: if (_fileSystem.FileExists(filePath))
8: {9: throw new IOException(@"An error occurred which did not allow the uploading of your file. " +
10: @"Please try again or contact your account manager.");
11: }12: if (!_fileSystem.DirectoryExists(directoryPath))
13: { 14: _fileSystem.CreateDirectory(directoryPath); 15: } 16: 17: if (creativeFile.GetFileExtension() == ".swf")
18: {19: // TODO: Put some type of check to make sure this is really a flash file.
20: _fileSystem.WriteBinaryFile(creativeFile.Bytes, filePath); 21: }22: else // Is not Flash Type (Image)
23: {24: try
25: {26: // THIS CALL FAILS SINCE ImageOrFlashUpload and Server are both undefined here
27: Ardalis.Framework.Drawing.Image.FormatAndSaveUploadedImage( 28: ImageOrFlashUpload.PostedFile, 29: expectedFormat.Height,30: expectedFormat.Width, false,
31: filePath,32: Server.MapPath("/images/admin/blank_logo.gif"));
33: }34: catch (Exception ex)
35: {36: throw new ApplicationException(
37: string.Format(
38: @"There was a problem uploading your image. Please verify that the width
39: height of the image you're uploading are exactly {0} x {1} pixels.", 40: expectedFormat.Width, expectedFormat.Height), ex); 41: } 42: } 43: }Now we need to refactor this library call so that it doesn't depend on an HttpPostedFile, then add some tests, and we're done (finally). What this code does is determine the dimensions of the uploaded image and verify they're what's expected, throwing an exception if not. To get this to compile for now, we can replace the call with the same thing that we used for .swf files:
1: _fileSystem.WriteBinaryFile(creativeFile.Bytes, filePath);Summary
In this part, we extracted an ugly method into its own class and along the way wrote some tests and added some properties and methods to our IFileSystem and WindowsFileSystem components. The end result is a testable CreativeFileStore (we haven't written enough tests yet - I decided this was getting long enough so they'll come in the next part) that (currently) is missing a little functionality it had before (verifying image dimensions) but which we'll work on restoring in the next step.
Comments
Tjipke said on 11 Dec 2008 at 4:09 AM
Hi, I must confess I didn't read all your posts about this subject, from first to last.
But I do have suggestion: there is already an abstraction for file content: Streams. So I would suggest that you could replace two of your methods with stream methods, to make your interface even more generic?
Stream LoadStream(string path);
void SaveStream(Stream bytes, string savingPath);
(Or maybe even generic:
Stream CreateStream(string path [, FileMode,...]))
LoadAllText could then be made a extension method, using a StreamReader?)
As I said just a suggestion, that came up while reading this..
members.home.nl/.../great_minds.htm
ssmith said on 11 Dec 2008 at 9:55 PM
I used streams in my spike solution (Part 2). I went with the byte[] array mainly due to its simplicity, but I certainly can see adding stream support to IFileSystem as well. With methods to convert from/to a stream/byte[] the choice is pretty arbitrary, with streams being somewhat more efficient but sometimes more difficult to work with, IMHO.
Tjipke said on 12 Dec 2008 at 2:33 AM
Hi,
Somehow in my previous comment a link (members.home....) was put in at the end, that should have been targetted at a different window on my screen... :-( So it has nothing to do with the comments above it. Sorry about that.