Ayende @ Rahien

Unnatural acts on source code

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

Brian Chris
04/11/2007 03:11 AM by
Brian Chris

"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?

Ayende Rahien
04/11/2007 03:26 AM by
Ayende Rahien

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?

Dan Maharry
04/12/2007 10:33 AM by
Dan Maharry

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

Siggie
04/12/2007 10:57 PM by
Siggie

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...

does adding hundreds of classes, each implementing a few lines >>of code, really make sense?

Yes, most definitely.

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 ?

Vitaly
04/13/2007 05:52 AM by
Vitaly

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 SRSupplementer(IEnumerable<string> applicableStates, IEnumerable<string> documents)

  {

       this.documents = documents;

      this.states = new List<string>(applicableStates);

  }


 public void Supplement(Policy policy)

 {

       if (states.Contains(policy.StateCode))

        {

               foreach(string document in documents)

               {

                      policy.AttachDocument(document);

               }

        }


 }

}

public class LedgerSupplementer : ISupplementDocuments

{

 public LedgerSupplement (int minLedger, IEnumerable<string> documents) { // set the ledger field & documents}


 public void Supplement(Policy policy)

{

      if (policy.LedgerAmount > this.minLedger)

      {

           foreach (string document in documents)

          {

              policy.Supplement(document);

         }

      }

}

}

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

{

protected BaseSupplementer (Predicate<Policy> trigger, IEnumerable<string> documents)

{

// ...

}

public void Supplement(Policy policy)

{

  if (this.trigger())

 {

       foreach(string document in documents)

       { // attach documents ... }

 }

}

}

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.

Ayende Rahien
04/15/2007 11:39 PM by
Ayende Rahien

@Dan,

On my todo list, I hope to get to it shortly.

Comments have been closed on this topic.