5

In any programming task, my preference is to write fail-fast code. That doesn't seem to be too controversial. However, I've also seen many developers say that constructors should do as little as possible. I'm finding that these two goals are often at odds, and conversations over fail-fast design vs. constructor simplicity sometimes devolve into statements of preference.

Consider a class that provides write access to a file. The class accepts a file path in its constructor which must be a path to an existing file. Some developers would say that a constructor shouldn't be accessing the file system, so I might just do a null check:

private readonly string file;

public FileAccessProvider(string file)
{
    this.file = string.IsNullOrEmpty(file)
        ? throw new ArgumentException(nameof(file)) : file;
}

Now, if the provided file doesn't exist, we might not know about it until the class actually attempts to write data to the file. In this case, would it be acceptable to do a check for file existence in the constructor?

private readonly string file;

public FileAccessProvider(string file)
{
    this.file = string.IsNullOrEmpty(file)
        ? throw new ArgumentException(nameof(file)) : file;

    if (!File.Exists(file))
    {
        throw new ArgumentException("File does not exist.");
    }
}

This complicates my constructor, but better adheres to fail-fast design. We could even take this a step further and add code to the constructor that checks if the user has write permissions to the file.

What about a class that has to access a database or network resources?

Is there an accepted side to err on in situations like this?

3
  • 1
    For reference: softwareengineering.stackexchange.com/q/224063/51654
    – Telastyn
    Jan 24, 2020 at 23:06
  • It does occur to me now that my hastily written, contrived example is a poor one here. I was more interested in the general consensus on fail-fast design and if it's at odds with keeping constructor code as simple as possible. Jan 25, 2020 at 0:00
  • 1
    How many hours per month those null pointer checks save you? And how much time writing and testing them (you do test them, right?) takes?
    – StaceyGirl
    Jan 25, 2020 at 10:43

7 Answers 7

8

Let me step away from your actual File I/O example, as you have noticed by yourself, this introduces aspects to your question you did not intend to ask here.

My answer to your general question is: don't take the idea "limiting constructor logic" too literal!

If the constructor of an object cannot initialize the object properly, throwing an exception is the standard way (and sometimes the only reasonable way) of signaling this to the caller. And if the decision about the initialization being successful or not requires to run a some more or less complex validation logic inside the constructor, that is fine, at least in most cases. That is usually the kind of "logic" which is acceptable inside a constructor.

Of course, there are complex cases where a two-step initialization of an object might be preferable, where the constructor does only some basic initialization and a separate Init method is required for the "heavy lifting". But the reason for such is design should be never a braindead rule like "no work inside the constructor". A sensible reason for such a design could be, for example, if you need to have the basic object construction done in a factory, and the Init method is a virtual method which is called as part of a template method.

See also: When is it right for a constructor to throw an exception?

11

I deeply believe in fail fast myself. In addition to promoting system integrity it helps debugging devs by getting them as close to the cause of the issue as possible so they don't have to slog through file after file trying to figure out where a bad value came from.

However, file I/O is not something you should pretend you control. You don't. You're never going to. Say you test that the file exists and then try to use it. Well then Murphy will wait until your code is deployed on a critical system to delete, rename, or move that file between when you tested it and when you attempted to use it.

Every time you use a file, suspect that it might not be there and be ready for it. Nothing you do before hand absolves you of this. It's simply the reality of how I/O works. Don't pretend Murphy won't do this because your app is the only one on the system. If Murphy has to he'll take the form of a rat that gnaws through your data cable.

On top of that, why throw an error if the file doesn't exist? Maybe this is a chance to make the file exist without hassling the user. Fail fast doesn't mean be pessimistic.

If the system is c:\MS\Windows and the user is typing /dev/nul you could be nice and parse their input and realize it's unusable. This is validation. It's the ONE thing constructors should do besides set instance variables. It's not about the current state of the file system. It's about whether your input is crap.

Validation can't involve talking to peripherals. In fact the thing you're validating must be full blown immutable or the test is a waste of time.

Fail fast should never overturn this. Fail fast argues that once attempting to use the file has caused an error you should get that exception going now, not later. But also, not before.

1
  • Yes, "system integrity" is a nice way to summarise it. Jan 26, 2020 at 15:09
1

I think you have two very different cases here:

  • A null-check. This should throw as soon as possible, because it is a bug in your program. That string will never become non-null, there is no way to recover or change. This is just wrong and should be caught immediately. There is no point in continuing with this broken class if the file is null. Please throw and let me know immediately that something is wrong before I have to figure out why it all went to pieces way further down the call chain.

  • A check for other, external dependencies. Those are not good checks anyway because they are pointless. The state of the file in the constructor says nothing about the state at the time of the call of the actual method. Lets say the file exists and you have write access. That could have changed by the time you make the actual call to the file writing method. So the check is pointless and superfluous. I have to handle exceptions on the actual method anyway, so please don't make me handle it in two places.

This pattern is used in .NET all over the place. For example while all the file handling methods in C# will throw an ArgumentNullException immediately upon null input data, there are no methods to easily check if you have write access. It does not make sense to check beforehand. You will see it when you actually try to write to the file. Knowing that you had write access two minutes ago is not useful at all.

0

I have never heard of fail fast as a software design principle. I view fail fast as a project or organisational strategy where one wants to learn of project failures as early as possible so one can course correct or abort while limiting the cost of having gone in the wrong direction. It's a risk mitigation strategy when there are a lot of unknowns.

I will also propose a goal of doing work as late as possible, i.e. when it is needed. In your example, I would not check if the file exists or if the name is plausible until a method accessing the file is called. In real-world examples (when classes do more than one kind of work) that time may never arrive.

Maybe it's due to the nature of your example being simplified - I would not do any of these checks. The framework will throw ArgumentNullException or FileNotFoundException (or SecurityException) on it's own.

The benefit of structured exception handling is so one can focus on the happy path throughout and error messages can be passed up the stack without every layer having error checks.

4
  • 3
    Regarding your first sentence, see ieeexplore.ieee.org/document/1331296 for an important paper on fail fast in software design, published in 2004 by IEEE. Jan 25, 2020 at 0:30
  • Learned something new. Thanks!
    – Martin K
    Jan 25, 2020 at 8:58
  • @ChristianHackl I prefer this formatting: martinfowler.com/ieeeSoftware/failFast.pdf
    – JimmyJames
    Jan 27, 2020 at 15:05
  • @JimmyJames: Yeah, I've never really understood why the IEEE website itself doesn't provide such a nice PDF. I just prefer references to primary sources, and I expect the IEEE URL to remain valid virtually forever. Jan 28, 2020 at 5:22
0

Seems like the provider class should expose a method that actually does the providing and error checking.

Then your class and constructor can be nice and simple (or just take in whatever other dependencies are needed) and not have any side effects. This also lets you add other methods to the class to support different data types or functionality without needing to have constructor overloads or other refactoring.

If I was needing to instantiate this provider, I wouldn't expect it to start making logical choices until I told it to go do something, via a method.

Perhaps better would be:

public class FileAccessProvider
{
    public string Get(string file)
    {
        this.file = string.IsNullOrEmpty(file) ? throw new ArgumentException(nameof(file)) : file;

        if (!File.Exists(file))
        {
            throw new ArgumentException("File does not exist.");
        }

        //get object

        return object;
    }
}
0

In this case, would it be acceptable to do a check for file existence in the constructor?

No.

Frankly, it's not acceptable to do the null check in the constructor.

(Why? Because very few people know what the behavior is when you throw from a constructor. Is the object created? Can you use it safely? What about if the constructor is in the middle of a type hierarchy?)

Worse, this check (along with the permissions check) are ineffective. Just because the file exists now and you have permissions now doesn't mean that it will exist with the right permissions when you try to use it (and vice versa).

Is there an accepted side to err on in situations like this?

In C# and similar languages, if your simple object creation can fail you'd make a static function that would do the cursory checks (path is nonnull) and return a new object. That would then have enough logic to fail fast when you're sure it will actually fail.

5
  • I understand that in some languages like C++, throwing an exception from a constructor is not acceptable, since it will cause a memory leak. In C# however, I don't believe that's possible. Can you elaborate on why it's unacceptable to perform simple validation like null checks in a constructor? Most answers I've read regarding this topic suggest otherwise (softwareengineering.stackexchange.com/questions/51062/…). I do agree that checking for file existence is a bad idea in my (unfortunately contrived) example. Jan 24, 2020 at 23:56
  • @AlanShearer: Throwing exceptions in constructors does not cause memory leaks in C++. Jan 25, 2020 at 0:19
  • I completely disagree with null check with you, it is just a special kind of invalid argument, the same one as for example providing negative number for the volume. As for people factor there are doc comments for that. As for static method I use them only when it is legal for me not to create object and return null, and I won't change it, because simply put there is enough thrower constructors, just checked List<T> from Microsoft, thrower after thrower and in all those years I didn't hear a complain that constructors from MS can throw an exception. Jan 25, 2020 at 5:40
  • @AlanShearer, memory leak is not possible, but resource leak is very possible. Say you create two IDisposable variables within constructor, when the second throws an exception you are in trouble. However! -- this problem is not specific to just constructors, you simply have to deal with it, constructors are just a tiny special case here, because the "main" object is not created here, so you cannot call Dispose on it. The solution is to use internally some kind of stack manager of disposables which automatically disposes previously created objects if any new in chain fails. Jan 25, 2020 at 5:44
  • 7
    I disgree to the first part of this answer, it actually says "never throw exceptions in constructors". But exceptions are actually a standard way to signal a failed initialization to the caller, and often it is the only reasonable choice. And any educated C# developer should not try to use an object after the constructor failed.
    – Doc Brown
    Jan 25, 2020 at 9:05
0

Late to the party but one option here that allows you to keep the constructor simple and also put logic around creation of the object is to use a static factory method.

By hiding the constructor, you can provide various, clearly named ways to create a new object. Any validations or computations that need to be done are placed in the body of the factory method and the constructor.

This is especially useful in cases where you want to create immutable objects (which should be the default choice, IMO.) It tends to be easier to create immutable objects when the constructor is simple. I find I have been able to eliminate the need for 'init' methods that set values after the constructor using this approach.

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.