Refactoring the DailyWTF
It is not always that I quote the Daily WTF as an example of good code, but today's article is very interesting. Alex is talking about hard coding vs. soft coding. Where as hard coding means moving things into code, and soft coding means moving things into configuration, rule engines, business integration layers, etc.
I have seen systems whose configuration literally dwarves the complexity of the system itself. The system can literally do everything. My canonical example to this is that I once need to build a program that reads a file and send it to a web service. The program should have been generic, so it was. At a boring integration session, I configured it to be a web server as well, just because I could.
The example given for bad code in the Daily WTF article is this:
private void attachSupplementalDocuments()
{
if (stateCode == "AZ" || stateCode == "TX") {
//SR008-04X/I are always required in these states
attachDocument("SR008-04X");
attachDocument("SR008-04XI");
}
if (ledgerAmnt >= 500000) {
//Ledger of 500K or more requires AUTHLDG-1A
attachDocument("AUTHLDG-1A");
}
if (coInsuredCount >= 5 && orgStatusCode != "CORP") {
//Non-CORP orgs with 5 or more co-ins require AUTHCNS-1A
attachDocument("AUTHCNS-1A");
}
}
And you know what, this isn't that bad of a code, certainly better than the alternatives paths that Alex has shown. It is still not good code, however.
The main issue that I have is that this method violates the SRP principal. I have three separate rules in this method, and three separate reasons to want to modify it. If given such a task, I would have refactored it to:
public interface ISupplementDocuments
{
void Supplement(Policy policy);
}
Where each if statement in the above code is a class that implements this interface. Now, the attachDocuments method looks like:
private void attachSupplementalDocuments()
{
foreach(ISupplementDocuments supplementer in this.supplementers)
supplementer.Supplement(this);
}
Now I can make use of IoC in order to gain both benefits, clear cut code, and flexibility in deployment and runtime. The end result is well factored code, that can be easily understood and worked on in isolation.
Comments
"Where each if statement in the above code is a class that implements this interface"
Having read Alex's article, I think you missed his point. You're attempting to Soft Code a non-problem. Even a simple system could have hundreds of business rules like this -- does adding hundreds of classes, each implementing a few lines of code, really make sense?
Yes, most definitely.
Small classes are very easy to understand, do you really want to trace through hundreds of business rules that may have dependencies manually?
Or try to follow a method with a hundred ifs?
Hi,
Your solution makes sense, but not being a great coder, how do you create and populate the collection of classes that I'm guessing this.supplementers is?
Thanks, Dan
All I can say is - if I would have used a Policy class for each "if" I use, my gut feeling is I would be lost in all the classes I would end up using .... Policies seem to me as different from conditions, and it just looks like you're scratching the walls to escape from classic-imperative style programming, but I might be wrong...
Well, never really tried the approach to throw it out the window, but I spontaneously twitch just thinking about it...
Could you give a code snippet of how the refactored code would end up looking like ? How generic or specialized will you Policy classes be ?
I agree with Ayende. I don't think the ISupplementDocuments implementers need to be too fine grained, but they should encapsulate the core logic check. In the case of the 1st "if" statement, certain requirements need to be met in order to attach a list of "SRxxx" documents. One way to implement that is something like this:
public class SRSupplementer : ISupplementDocuments
{
}
public class LedgerSupplementer : ISupplementDocuments
{
}
The first class can be populated with the list of SRxxx documents, and the core business logic applicable to this class of documents (i.e. checking for certain states) is encapsulated here. The second class works similarly, except you're checking against some integer to see if it meets the minimum requirement. In fact, you can even start to see some commonality between the two supplementers: each takes a list of documents in its constructor that it then uses to supplement the policy with. If you take it a step further, you can presumably define a base ISupplementDocuments class as follows:
public abstract class BaseSupplementer : ISupplementDocuments
{
{
// ...
}
public void Supplement(Policy policy)
{
}
}
In the latter case, derived classes just have to provide a delegate that will evaluate the Policy against come condition, and then attach the documents. It's really the same thing as each child class performing the check internally, so how you implement it (using a delegate or encapsulated method) is a personal design choice.
The main idea here is that you capture the specific business logic (in this case, the "if" conditions) into separate classes that can then change/evolve on its own. The core Policy class can stay the same, and new business rules/documents can be added by simply creating new classes that encapsulate a certain classification.
At the end of the day, the additional code for each "if" statement is either going to live in this one monolithic method (the original version) or you're going to externalize it into smaller, more maintanable classes, but the code will reside SOMEWHERE. So yes, you're creating additional classes, but you're actually making your system more maintainable and extensible in the process.
@Dan,
On my todo list, I hope to get to it shortly.
Comment preview