5
\$\begingroup\$

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:

  1. Omit interface, make FileIoProvider methods virtual, so they can be overridden for mocking.
  2. 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 */
    }
}
\$\endgroup\$
  • \$\begingroup\$ Note: updated code, original still had System.IO instead of _ioProvider calls in "testable" FileWithIndexSaverBloated class. \$\endgroup\$ – Erik Hart Dec 19 '15 at 15:29
7
\$\begingroup\$

You could write your own System.IO interfaces and use them to wrap the classes from the namespace, but why reinvent the wheel? Go install the System.IO.Abstractions nuget package in your project and call it a day.

\$\endgroup\$
  • \$\begingroup\$ I'm going to check this one out. It would be great to also have a solution for when using the FileInfo and DirectoryInfo classes as parameters. These seem hard for me to test without using verbose wrapper classes. \$\endgroup\$ – Erik Hart Dec 20 '15 at 16:55
  • \$\begingroup\$ They're in there @ErikHart. github.com/tathamoddie/System.IO.Abstractions/tree/master/… \$\endgroup\$ – RubberDuck Dec 20 '15 at 16:58
6
\$\begingroup\$

When now increasingly making code unit testable, I notice that code gets bloated, only to get dependencies out.

Getting dependencies out of your code is a good thing, because it makes it wider reusable.

This at least weighs out writing a little bit of more code for introducing the interfaces.


At least it seems to me you're following the wrong approach with your code. It might probably be better to separate the functionality of building the path and saving the file.

\$\endgroup\$
5
\$\begingroup\$

This is not a safe way to ensure that no existing file gets overwritten when creating a file:

while (File.Exists(newFileName))
{
    index++;
    newFileName = pathNameNoExt + "_" + index + dotExtension;
}

using (var writer = new StreamWriter(File.Create(newFileName)))
{
    …
}

The problem is that there is a race condition: a file of the same name could spring into existence after File.Exists(newFileName) reports that there is no such file. As the documentation for File.Create(newFileName) says:

If the specified file does not exist, it is created; if it does exist and it is not read-only, the contents are overwritten.

The race condition might happen accidentally due to multithreading, or it might be triggered deliberately by an attacker, perhaps aided by a debugger that pauses execution before File.Create().

What you should use is the FileStream(path, mode) constructor with FileMode.CreateNew as the mode:

Specifies that the operating system should create a new file. This requires FileIOPermissionAccess.Write permission. If the file already exists, an IOException exception is thrown.

Catch the IOException, and if the file already exists, increment the filename and try again.

\$\endgroup\$
1
\$\begingroup\$

This is not really a code review, but it's too long for a comment, so I'll just leave this here.

When you use this IIoFileProviderBloat interface, you can test for exceptions that could (and probably will) happen when accessing something as volatile and stateful as the file system. What happens if you try to create a file that does not exist, in a folder that you don't have any write access to? Or to a drive that's been mapped to a network share, which won't respond (unless you count that under 'network resource'; I assumed you meant that you don't foresee the need to create a different implementation for network resource access).

Also, implementing the FileWithIndexSaverBloated as a non-static class means you can mock it out if you are going to run tests on other classes. If you want to run tests that create large files and which will need to call this method, they will run slower. And slower tests are no fun to run. And tests that are no fun to run, will be skipped. And skipped tests will not find regression bugs.

I agree, using Path will probably not need to be mocked out, as those methods are "pure" (they have no side effects, don't depend on external state and always return the same thing for the same input). So you might want to leave those out. Unless you foresee the one external thing changing that they do depend upon: the OS that they are written for.

One final thing: if you're going with the IIoFileProviderBloat way (which I'd recommend), I suggest you do change the name.

\$\endgroup\$
  • \$\begingroup\$ Check my latest edit, which has the class with just minimum change, with CheckIfExists(...) and Save(...) as protected virtual methods and a static SaveFileWithNumberIfExisting(...), instantiating the no longer static FileWithIndexSaverTestable class on-the-fly. An internal method allows insertion of derived class instances (e.g. mocks). This can still simulate all the weird errors that can occur with a file system. Network drives (not network resources like FTP, WebDAV etc.) may be another problem, the method was designed with a local, or at least nearby drive, in mind. \$\endgroup\$ – Erik Hart Dec 20 '15 at 21:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.