Methods which wrap a single method.

Question

I think I'm going mad, someone please reassure me.

public class MyFile
{   
    public static byte[] ReadBinaryFile(string fileName)
    {
        return File.ReadAllBytes(fileName);
    }

    public static void WriteBinaryFile(string fileName, byte[] fileContents)
    {
        File.WriteAllBytes(fileName, fileContents);
    }
}

People keep on adding code like the above in to our code base, surely this is wrong and horrid and I am doing the world a favour by deleting it and replacing all (or both in this case...) references to it with the internal code.

Is there any real justification for this kind of thing? Could I be missing the bigger picture? We are quite YAGNI-centric in our team and this seems to fly in the face of that. I could understand if this was the beginnings of something more, however this code has lay dormant for many many months until I tripped over it today. The more I search the more I find.

Answer 1

No. It's silly. Please make it stop.

Answer 2

The existence of those methods is a disgusting aberration that should be corrected immediately.

They were probably written by someone who didn't know about the File class, then rewritten by someone who did, but wasn't as daring as you.

Answer 3

I guess the answer to this does depend on your team, practices and what the codes ultiamte purpose is for (eg. The piece of code you've found currently writes to a file but will be writing to a web service/database/morse-code machine once it's finished - although that "excuse" is kinda defeated by the class/method names). I think you've answered the question yourself with "We are quite YAGNI-centric in our team and this seems to fly in the face of that".

The ultimate answer though would be to ask the person who wrote it why they wrote it that way.

Answer 4

As written, the class/methods are garbage. However, I can see a situation in which a similar pattern might be used legitimately:

public interface IFileStorage
{
    byte[] ReadBinaryFile(string fileName);
    void WriteBinaryFile(string fileName, byte[] fileContents);
}

public class LocalFileStorage : IFileStorage { ... }

public class IsolatedFileStorage : IFileStorage { ... }

public class DatabaseFileStorage : IFileStorage { ... }

In other words, if you wanted to support different kinds of storage, then you might actually wrap very simple methods in order to implement a generic abstraction.

As written, though, the class doesn't implement any interface, and the methods are static, so it's pretty much useless. If you're trying to support the above pattern, then refactor; otherwise, get rid of it.

Answer 5

It is rather silly, until you think about hiding the implementation details of those methods.

Take for example if you had code like this

File.WriteAllBytes(fileName, fileContents);

scattered throughout your code, what if some day down the line you wanted to change your application's method of writing a file out? Well in that case you would have to go throughout your code and update all of these lines of code to adopt the new method, where as with the above version you would only need to change it in one place.

I am not saying their way is right and you are wrong to correct it, I am just adding some perspective

Answer 6

If all the methods do is take the same parameter list, and pass them through, then no, it is pointless, and I think actually makes the code less understandable. However, if the method also passes on default values besides the ones passed as parameters, or does some sort of common validation on the parameters, then it's a harder argument to make.

Are these methods placeholders for later logic to be added? If so, then comments or even the dreaded TODO statement should be added so that someone wraps around later and completes the thought.

Answer 7

I don't think these methods make much sense as static methods of a helper class, but the pattern has merit in some cases. For instance:

1. To decouple code from a static class. If MyFile was not static then it might serve as an abstraction over the static IO.File object. Code that directly accesses the filesystem can be very difficult to test, so abstractions like this can be useful. (For instance, I have an IFileSystem interface that all my I/O code depends on, and my FileSystem class implements this interface and wraps the basic methods of File.)

2. To preserve consistent levels of abstraction in a method. I don't like to mix code in the problem domain (e.g. "customers" and "orders") with code in the _solution domain) (files and bytes and bits). I'll often write wrappers like this to make the code easier to read and to give me extensibility points. I'd rather read GetDataFileContents() than File.ReadAllBytes("foo.dat").

3. To provide context. If a piece of code is being executed for its side effects, such as deleting a text file to effect the deletion of a customer's order, then I'd prefer to read DeleteCustomerOrderFile("foo.txt") than File.Delete("foo.txt"). The former provides contextual information to the code, the latter does not.