return (Number == 0 && Locked) || (Orders[Number].IsComplete);
Because the number can be either related to a quote or an order we need to check the quote being locked or the order being complete. The subtlety I had missed is what happens when you pass it a number greater than 0 which is not actually an order and what happens if you pass in 0 but the quote is not locked. In both cases, the number is passed to the indexer for orders which then throws an exception. This can be fine but how can you ensure the potential exception is considered and caught or make sure the function is not called for invalid values?
You can resolve most of the issues with the following:
- Make the function as private as you can, this way only the class it lives in or possibly subclasses has to be concerned with its correct use. Don't get in the habit of making all functions public just to avoid thinking about it.
- Consider whether it is correct to put a guard in the function generally that might only call the logic if a precondition is true such as the order exists etc. This could either be around the logic or part of the logic itself.
- Check the logic carefully. In the above example, I should have checked the Number being greater than one in the second set of brackets since orders start from 1 and if the quote is not locked, the second clause would be evaluated for 0 which would then throw.
- Ask whether the functions can be pushed further down into the system or refactored to hide them from logic problems. For instance, instead of passing in an int which then has to work out what it is, pass in an object of some interface or super class that you can then call a function on, this way you have no knowledge of the inner workings or logic of the numbering scheme and no chance to muck it up!