Ayende @ Rahien

Ayende Rahien commented on Refactoring the DailyWTF

Sun, 15 Apr 2007 23:39:05 GMT

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

Vitaly commented on Refactoring the DailyWTF

Fri, 13 Apr 2007 05:52:33 GMT

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 applicableStates, IEnumerable documents) { this.documents = documents; this.states = new List(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 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 trigger, IEnumerable 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.

Siggie commented on Refactoring the DailyWTF

Thu, 12 Apr 2007 22:57:57 GMT

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 ?

Dan Maharry commented on Refactoring the DailyWTF

Thu, 12 Apr 2007 10:33:39 GMT

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

Ayende Rahien commented on Refactoring the DailyWTF

Wed, 11 Apr 2007 03:26:42 GMT

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

Brian Chris commented on Refactoring the DailyWTF

Wed, 11 Apr 2007 03:11:01 GMT

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