When now increasingly making code unit testable, I notice that code gets bloated, only to get dependencies out. A simple file operation, like saving a file, and, if a file with the name already exists, append a numeric index, can become greatly bloated by this.
I understand that writing real files in a unit test is not a good choice, but isn't it better to avoid too much code bloat for decoupling? Also, the simple version can be static, while the testable one requires instantiation.
The function is likely to never save anything other than files, never any database entry, network resource or whatever.
/// <summary>
/// Save file with given name, or with underscore and next free numeric ending,
/// if already existing. Bound to System.IO file operations.
/// </summary>
public static class FileWithIndexSaverSimple
{
public static string SaveFileWithNumberIfExisting(string pathName, string content)
{
string pathNameNoExt = Path.Combine(
Path.GetFullPath(pathName),
Path.GetFileNameWithoutExtension(pathName));
string dotExtension = "." + Path.GetExtension(pathName);
string newFileName = pathNameNoExt + dotExtension;
int index = 0;
while (File.Exists(newFileName))
{
index++;
newFileName = pathNameNoExt + "_" + index + dotExtension;
}
using (var writer = new StreamWriter(File.Create(newFileName)))
{
writer.Write(content);
writer.Flush();
}
return newFileName;
}
}
The decoupled version, with two classes and IIoProvider interface, is much too long for my taste, compared to the simple version with System.IO dependency.
/// <summary>
/// Unit testable version
/// </summary>
public class FileWithIndexSaverBloated
{
private readonly IIoProviderBloat _ioProvider;
public FileWithIndexSaverBloated()
{
_ioProvider = new FileIoProviderBloat();
}
/// <summary>
/// Mocking/testing only
/// </summary>
/// <param name="ioProvider">Any IIoProvider, mock of IIoProvider or whatever</param>
internal FileWithIndexSaverBloated(IIoProviderBloat ioProvider)
{
_ioProvider = ioProvider;
}
public string SaveFileWithNumberIfExisting(string pathName, string content)
{
string pathNameNoExt = _ioProvider.GetPathNameBeforeExtension(pathName);
string dotExtension = "." + _ioProvider.GetExtension(pathName);
string newFileName = pathNameNoExt + dotExtension;
int index = 0;
while (_ioProvider.CheckIfExists(newFileName))
{
index++;
newFileName = pathNameNoExt + "_" + index + dotExtension;
}
_ioProvider.Save(newFileName, content);
return newFileName;
}
}
/// <summary>
/// Required to mock
/// </summary>
public interface IIoProviderBloat
{
bool CheckIfExists(string ioItemName);
string GetPathNameBeforeExtension(string ioItemName);
string GetExtension(string ioItemName);
void Save(string ioItemName, string ioContent);
}
/// <summary>
/// Wrap .NET System.IO
/// </summary>
public class FileIoProviderBloat : IIoProviderBloat
{
public bool CheckIfExists(string pathName)
{
return File.Exists(pathName);
}
public string GetPathNameBeforeExtension(string pathName)
{
return Path.Combine(
Path.GetFullPath(pathName),
Path.GetFileNameWithoutExtension(pathName));
}
public string GetExtension(string pathName)
{
return Path.GetExtension(pathName);
}
public void Save(string pathName, string content)
{
using (var writer = new StreamWriter(File.Create(pathName)))
{
writer.Write(content);
writer.Flush();
}
}
}
Is there any best practice how to reduce this?
My ideas would be:
- Omit interface, make FileIoProvider methods virtual, so they can be overridden for mocking.
- Keep only the FileWithIndexSaver class, add the methods from FileIoProvider as protected virtual to it, override them in mock class.
At the moment, I am not sure how RhinoMocks (which I currently use) work with classes that have only a part of their methods virtual, but I can even create a specific override implementation for the test (no mock).
The methods which only select parts of the path (with .NET Path class) can even remain as dependencies, because they don't rely on an actual file system.
Here's a slim, testable version, created with the intention to get rid of the real file system access only. It even has the static behavior preserved.
I am not sure if it is mockable with the common mocking frameworks, but one can easily create a mock class on his own, like the example.
I agree, once this saving method extends to resources other than .NET File IO, then the IIoProvider interface will become necessary, same for other algorithms of indexed name creation. But for now, that seems zealous for me, and I prefer to stick to YAGNI/You aint gonna need it (now).
/// <summary>
/// Save file with given name, or with underscore and next free numeric ending,
/// if already existing. Dependency on actual file access overridable.
/// </summary>
public class FileWithIndexSaverTestable
{
public static string SaveFileWithNumberIfExisting(string pathName, string content)
{
return SaveFileWithNumberIfExisting(
pathName, content, new FileWithIndexSaverTestable());
}
internal static string SaveFileWithNumberIfExisting(
string pathName, string content, FileWithIndexSaverTestable instance)
{
string pathNameNoExt = Path.Combine(
Path.GetFullPath(pathName),
Path.GetFileNameWithoutExtension(pathName));
string dotExtension = "." + Path.GetExtension(pathName);
string newFileName = pathNameNoExt + dotExtension;
int index = 0;
while (instance.CheckIfExists(newFileName))
{
index++;
newFileName = pathNameNoExt + "_" + index + dotExtension;
}
instance.Save(newFileName, content);
return newFileName;
}
protected virtual bool CheckIfExists(string pathName)
{
return File.Exists(pathName);
}
protected virtual void Save(string pathName, string content)
{
using (var writer = new StreamWriter(File.Create(pathName)))
{
writer.Write(content);
writer.Flush();
}
}
}
A mock class in a test could look like this (if not directly using mock frameworks):
public class FileWithIndexSaverTestableMock : FileWithIndexSaverTestable
{
public static string SaveFileWithNumberIfExisting(string pathName, string content)
{
return FileWithIndexSaverTestable.SaveFileWithNumberIfExisting(
pathName, content, new FileWithIndexSaverTestableMock());
}
protected override bool CheckIfExists(string pathName)
{
/* mock behavior here */
return false; /* replace by mock logic */
}
protected override void Save(string pathName, string content)
{
/* Mock save behavior here */
}
}
