CODEWITHSUNDEEP
c#.net
Jim Counts
Jim Counts

Reputation: 12805

What exception should I throw when a factory method returns null?

Consider a factory method that I may or may not control, passed to another class as a Func<T>:

// this.FactoryMethod is an external dependency passed into this class
// through constructor or property injection assume that it is not null
// but there is no guarantee that it will return a non-null reference.
Func<IModel> FactoryMethod;

public IModel GetPopulatedModel(int state, FileInfo someFile)
{       
   // Argument validation omitted for brevity...

   // This operation could return null.
   var model = this.FactoryMethod()
   if (model == null)
   {
      throw new SomeException("Factory method failed to produce a value.");
   }

   // Safe to assign to properties at this point.
   model.Priority = state;
   model.File = someFile;
   return model;
}

I would like to throw something that indicates the operation failed.

ArgumentNullException would be misleading because there is nothing wrong with the arguments:

The exception that is thrown when a null reference (Nothing in Visual Basic) is passed to a method that does not accept it as a valid argument.

InvalidOperationException doesn't really seem to be on point:

The exception that is thrown when a method call is invalid for the object's current state.

It seems weird to consider a local variable to be part of the object's current state.

What would be good exception to throw? I'm surprised there is no OperationFailedException in System. Should I write my own exception, or is there a good one out there?

Upvotes: 2

Views: 6304

Answers (9)

Nicole Calinoiu
Nicole Calinoiu

Reputation: 21002

NotImplementedException is definitely not the correct choice here. Its intended meaning is "not implemented yet", and it should pretty much never be thrown by production code.

The most important criterion for what to throw in this scenario is what would be most useful to the consumer(s) of the exception. If you expect that calling code might need to react differently to this exception than to any arbitrary exception, the exception type is very important. On the other hand, the only thing that calling code would ever do with such an exception is to log it, its message is very important, and its type is somewhat less important. In the latter case, an InvalidOperationException with an explicit message would probably be quite suitable.

It's also worth thinking about whether the return of the null value from the factory method should be considered a violation of its contract or not. The fact that you want to throw an exception in this case suggests that it probably should be, in which case you might want to change the design a bit so that an interface instance is injected instead of a Func<>. This would allow you to define either a documented contract on the interface method, specifying that it will never return null, and that it will throw an exception if it can't create a model instance. If you're using Code Contracts, you could even make the non-null return value part of its explicit contract.

On a last little note, if you think that NullReferenceException is an acceptable thing to throw here (which your response to another answer would seem to indicate), you can accomplish this by simply letting the model.Priority = state line execute. The only problem with this (which is shared by any other approach in which GetPopulatedModel throws an exception after receiving a null model instance from the factory) is that the source location of the exception isn't particularly helpful to a developer who is trying to figure out why the retrieved model was null. This is why "encouraging" the factory to throw its own exception might be the best candidate approach.

Upvotes: 2

Jim Counts
Jim Counts

Reputation: 12805

Another option: don't throw an exception. In the actual application, the method is called from within a try/catch block, and this exception would have been caught and logged. There was no advantage to throwing an exception in this case.

So instead of throwing the exception, I just logged the message and returned null. Translating this to the example code looks like this:

// this.FactoryMethod is an external dependency passed into this class
// through constructor or property injection assume that it is not null
// but there is no guarantee that it will return a non-null reference.
Func<IModel> FactoryMethod;

public IModel GetPopulatedModel(int state, FileInfo someFile)
{       
   // Argument validation omitted for brevity...

   // This operation could return null.
   var model = this.FactoryMethod()
   if (model == null)
   {
      this.Logger.Write("Factory method failed to produce a value.");
      return null;
   }

   // Safe to assign to properties at this point.
   model.Priority = state;
   model.File = someFile;
   return model;
}

If throwing an exception were required/useful, I would have gone with NotImplementedException, but in this case, not throwing an exception made more sense.

Upvotes: 0

dtb
dtb

Reputation: 217361

Two examples from the .NET Framework:

CloneCore is essentially a factory method for clones of the current object.

I would use a NotImplementedException, because the factory method does not implement what is requested from it.

Upvotes: 3

iMax
iMax

Reputation: 31

Why not use the InvalidOperationException() since you don't have the control on the Method used if it fail it means that it was invalid.

Upvotes: 1

Justin Pihony
Justin Pihony

Reputation: 67115

You can always create your own Exception if you feel that the available exceptions at your disposal are not adequate.

UPDATE Although, I do agree with @TomTom and @dtb as to the possible options that are out there. Especially the NotImplementedException as the factory is indeed not implemented

Upvotes: 5

reach4thelasers
reach4thelasers

Reputation: 26909

I'm not sure this is really the Factory pattern you're implementing here. But to answer your question, just make a custom exception as follows:

public class FactoryNullException : Exception
{
    public FactoryNullException() : base("Factory was null")
    {

    }
}

Upvotes: 0

ahmet
ahmet

Reputation: 646

Maybe

NullReferenceException

is what you want.

Upvotes: 0

TomTom
TomTom

Reputation: 62137

InvalidOperationException.

A local variable is what defines the object's state per definition. Nothing bad. The factory method must be set, or you can not create objects. InvalidOperation - factory can not create object at that moment because the factories internal state is wrong for that.

Upvotes: 2

Luis Filipe
Luis Filipe

Reputation: 8718

I would create my own exception called something like

FactoryNullException

Then you can even create the default message you want, e.g., the one in your example

Factory method failed to produce a value.

Upvotes: 2

Related Questions