2 Golden rules of using Exceptions:
- Fail Fast
- Exceptions should be... well exceptional.
These two rules are contradicting, or are they?
Fail fast dictates that we should throw an exception, when an operation is used on an invalid state.
public class Customer { private bool _authenticated; private Command _command; private Logger _logger; public Customer(bool authenticated, Command command, Logger logger) { _authenticated = authenticated; _command = command; _logger = _logger; } }
First of all, we dont let the ctor do anything besides assigning fields. It does not throw exceptions ever! So basically we allow our object to be created in a potentially invalid state.
It is first when an operation is used on an invalid state, that we have to fail fast. And since we dont ever do any operations on object in our ctor, it will never throw an exception.
Methods use fields and arguments in two ways. They pass on the reference to the object or they operate on the object. Only throw exceptions if the method operates on the object.
public class Customer { private bool _authenticated; private Command _command; private Logger _logger; public void PassOnLoggerToCommand() { //_command is operated on so throw if it is null //_logger is passed on as a reference, so do not throw. if(_command == null) throw new InvalidOperationException("command is null. Are you missing a dependency?"); _command.SetLogger(_logger); } }
If you throw an Exception from with in a method - not at the top of the method -, that is a good indication that you need to refactor the method into two.
public class Customer { private bool _authenticated; private Command _command; private Logger _logger; public void Execute() { if(!_authenticated) { //Exception throw inside the method (BAD) if(_logger == null) throw new InvalidOperationException("logger is null. Are you missing a dependency?"); _logger.Info("Attempted login..."); } else { //Exception throw inside the method (BAD) if(_command == null) throw new InvalidOperationException("command is null. Are you missing a dependency?"); _command.Execute(); } } }
Can be refactored to.
public class Customer { private bool _authenticated; private Command _command; private Logger _logger; public void Execute() { if(!_authenticated) { LoggerInfo("Attempted login..."); } else { ExecuteCommand(); } } private void ExecuteCommand() { //Exception throw at the top of the method (GOOD) if(_command == null) throw new InvalidOperationException("command is null. Are you missing a dependency?"); _command.Execute(); } private void LoggerInfo(string message) { //Exception throw at the top of the method (GOOD) if(_logger == null) throw new InvalidOperationException("logger is null. Are you missing a dependency?"); _logger.Info(message); } }
Exceptions should be exceptional, and there are performance implications of throwing an exception, so what to do?
Make a query method on the class that checks that a given operation can be made with the current state. So that you never have to catch Exceptions that you create in your own code.
public class Customer { private Command _command; public Customer(Command command) { _command = command; } public void ExecuteCommand() { if(!CanExecuteCommand) throw new InvalidOperationException("command cannot be executed given the current state.?"); _command.Execute(); } public bool CanExecuteCommand() { return _command != null } }
Try to make the queries simple questions that return a bool. You might find it that the queries also help when writing automated tests.
A final note, beware of threading issues when using queries.
4 comments:
Hey Morten,
I noticed you say not to "throw exceptions ever" in a constructor. Isn't it easier to validate command and logger and throw an Exception than set the private _command and _logger fields, then later have to throw InvalidOperationException when another method in the class detects _command and _logger are invalid?
For example, if the arguments "command" or "logger" are invalid, throw a new ArgumentNullReference. All the .NET classes do this (use Reflector to check). This forces the coder to check for null references before passing them into a construction (this should always be done), and eliminates the extra work of checking the state of the class later. Additionally, for example, new Regex() and XPathExpression.Compile() will throw exceptions if the argument string is not a valid regular expression or XPath expression, respectively.
Could you explain the logic behind "don't ever do any operations on an object in [the] ctor?" Arguments should always be validated before being used (e.g. assigning a private field to the value of an argument) in a method or constructor.
Maybe I didn't pay enough when I was following along with your post. :)
Chris
If the ctor needs to throw an exception when in invalid state, it need to know about all invalid state and all usages of the class.
It is much easier only to throw in a method, and check the current state of the object there.
Moreover when I do a new FooBar("test"), I dont want to put that in a try catch. Just imagine how many try catch you will have in a program.
If you dont throw in the ctor, you can use the query methods to check for the state.
psudo code:
1)create new
2)ask the object is it is valid to do something
3)and the do it.
If you check the state on an object that is only passed on as a reference, you are ensentially breaking encapsulation. Because you copy the post condition from the object that you pass the reference to. And if you ever need to change post condition you have to do it multiple places
I agree with the two axioms, Fail Fast and Exceptions should be exceptional.
However, I disagree on a couple of points.
You write:
Moreover when I do a new FooBar("test"), I dont want to put that in a try catch. Just imagine how many try catch you will have in a program.
I could use the same argument for methods: I don't wan't to put method calls in a try catch. Imagine the number of try catch statements! That's nonsense. In the lifetime of most objects, methods are called more often than the constructor, so if I use can try-catch blocks around method calls I can afford them around instantiation statements as well.
There's a different API design principle that says: Don't split initialization. If the user of your API creates an object she's in her right to expect that the object in fully initialized. Requiring her to do additional initialization before calling some methods leads to confusion and errors. The contract of the constructor and the user is: When I exit withour errors, this object is ready to be used.
Not being able to force constraints on fields inside the constructor leads you down a path where you have to implement double initialization which is an anti pattern.
I believe that throwing an exception in a ctor makes alot of sense! What if your object is created by another thread, image how hard it would be to track down who is initializing objects incorrectly...
Post a Comment