A few weeks ago I was looking at quite an interesting bug in our system which initially didn’t seem possible.
On one of our screens we have some questions that the user fills in which read a bit like this:
- Do you have a foo?
- Is your foo an approved foo?
- Is your foo special?
i.e. you would only see the 2nd and 3rd questions on the screen if you answered yes to the first question.
However, if you went to the next page on the form having answered ‘Yes’ to the first question and then came back to this page and changed your answer to the first question to ‘No’ then when the data got submitted to the server the answers to the other two questions would still be posted.
We were therefore ending up with a request with the following values submitted:
hasFoo "No" approvedFoo "Yes" specialFoo "Yes"
Later on in the application we had some logic which decided what to do with the user request and this one was erroneously being approved because we hadn’t catered for the condition where the first question was answered ‘No’ and the other questions had ‘Yes’ values.
In theory we should have written some client side code to reset the optional questions if the first one was answered ‘No’ but even then it’s still possible to post whatever data you want to the server if you try hard enough.
Given that we need to behave in a somewhat defensive way somewhere on the server.
My initial thinking was that perhaps we should change the logic to handle this scenario but talking through the problem with Mike he pointed out that it would make more sense to fix the data earlier in the chain.
I ended up writing some code in the controller to change the latter two fields to ‘No’ if the answer to the first question was ‘No’. We’re not really using custom binders on the project otherwise I think it would be the type of logic that would go in there.
Overall I’m no really a fan of defensive programming in general because it often seems to lead to over complicated code but in the case of user input it does make sense and the general guideline seems to be to fix any logically invalid values as close to the entry point of our application as possible.
Looks weird for me too. Maybe, in this case, you can show or not the other fields. Or change the behaviour of your domain object, like Steve Py said.
This isn’t defensive programming. It is input validation. Also, this validation should take place in a trusted part of the application, which is the server-side component.
Another option is to change the behaviour of your domain object.
I.e.
private bool _hasFoo;
public bool HasFoo
{
get { return _hasFoo;}
set { _hasFoo = value;}
}
private bool _approvedFoo;
public bool ApprovedFoo
{
get { return HasFoo && _approvedFoo; }
set { _approvedFoo = value; }
}
// etc.
It depends on the behaviour you’re after though, and coding practices you follow. (I.e. using property accessors consistently rather than private members directly.) The benefit here is that selections made before being deemed invalid can still be remembered by UI should a user go “whups! I didn’t mean to change that.” For a 1 or 2 selection graph it’s pretty much irrelevant, but when you’re dealing with trees of selections and options it can be beneficial. (If another option hinged on “Approved Foo” the check can cascade safely.)
Ultimately something like “Approved Foo and Special Foo are only applicable if you have a Foo.” needs to be recognized as a unit of business logic and handled/tested for accordingly.
“Overall I’m not really a fan of defensive programming in general because it … leads to overly complicated code”
Don’t take this the wrong way, but after reading the above statement my first reaction was, “well, then you’re doing it wrong!”. Defensive programming doesn’t have to mean “add a bunch of -if- statements that obscure my business logic”. I’ve had great success using a combination of extension methods, design-by-contract-style static helpers and normal “clean code” techniques. With a little bit of effort it’s entirely possible to practice defensive programming AND write clean, elegant code!