OdeToCode IC Logo

It's Design

Wednesday, January 26, 2011

If I come across this code:

if (widget.Price < 10.0m && widget.UnitsInStock > 100)
    // ... 

Then the first thing I'll suggest is refactoring to: 

if (widget.IsEligibleForDiscount)
    // ...

But It's Not Reusable!

"IsEligibleForDiscount will never be reused!", someone will shout. "Why do we go to the trouble if the logic is never reused?".

Ah, yes - but if we only use encapsulation as a means to generalize and achieve reuse, then we're not taking advantage of what classes, methods, functions, objects, and properties truly have to offer. As Kevin Henney once wisely said: "there is no such thing as reusable software, only software that has been reused".

But It's Trivial!

"Why go to the trouble of creating a one-line method or property?".

Yes, it is is trivial, but you can't measure the effectiveness of an abstraction by it's size,  or by the amount of effort required to create the abstraction. This is like measuring the impact of an animation by counting how many colors it uses. It's often the small, quick, subtle animations that improve usability and polish. Likewise, a good abstractions can be small and still improve the usability of the code.

But It's Bad For Performance!

Go away - and don't come back without hard numbers.

It's Design

You've probably worked on applications that have two types of objects - objects with data and object with behavior. The objects with behavior are mostly procedural scripts to manipulate the objects with data. Strings and integers dominate the codebase. The objects aren't part of an object oriented design, but part of a process decomposition.

Breaking out of design habits is hard - it takes deliberate practice, and depends on introspective moments.

We can debate in our minds if the refactoring make the code better or worse - but let's at least have the debate.

Gravatar Paul Hadfield Wednesday, January 26, 2011
Is it over-engineered to create an interface "IWidgetEligibleForDiscount" and add a method called "Determine" which takes an instance of a widget and returns true/false. You can then create a rule class that inherits from the interface that wraps up logic cleanly too. You could then easily swap out for a new rule, or even have one or more different rules checked for a single widget.
Gravatar Michael Silver Wednesday, January 26, 2011
It's a trivial change and it's easier to read and self documenting. Enough said.
Gravatar H&#252;seyin T&#252;fek&#231;ilerli Wednesday, January 26, 2011
A refactoring that I would do for this kind of code to improve readibility is this:

var isEligibleForDiscount = widget.Price < 10.0m && widget.UnitsInStock > 100;

if (isEligibleForDiscount)
// ...
Gravatar Eber Irigoyen Wednesday, January 26, 2011
it's about readability, if the property bothers them so much because it doesn't get used anywhere else, just use a local bool variable right before the if
Gravatar Liam McLennan Wednesday, January 26, 2011
It's interesting that you mention the procedural script anti-pattern. The fact that you have some code that is interrogating the internals of your widget suggests that there might be a bit of that going on here.

What if the code within the if statement was moved to a method on widget and the logic test was applied within that method? Then the top-level code becomes widget.DoSomething(). Tell, don't ask.

Gravatar TweeZz Thursday, January 27, 2011
A colleague of mine just made a similar (?) test for applying a small refactoring like this in javascript.
We were quite surprised with the results :)
Gravatar Mikkel Toudal Kristiansen Thursday, January 27, 2011
If would just refactor de boolean expression out into a mehod taking the widget as a parameter:

private bool WidgetIsEligibleForDiscount(Widget widget)
return widget.Price < 10.0m && widget.UnitsInStock > 100;

This makes the original "if" read like this:
if (WidgetIsEligibleForDiscount(widget)
// ...

This should also be a standard refactoring on most IDEs.

Just my 5 cents :-)
Gravatar Haripraghash Thursday, January 27, 2011
Definitely a case for the specification pattern!!
J.P. Hamilton Thursday, January 27, 2011
Just a crazy thought, but if it's just used once or twice, you could implement as an extension method within the namespace where it was being consumed. Otherwise, I would just do an Extract Method on it as Mikkel suggested.
G McCormick Thursday, January 27, 2011
How do you know it won't ever be reused? As the customer adds more functionality and enhancements? Maybe version 2 or 3 or 4...
Gravatar Dave Herren Thursday, January 27, 2011
I agree with you but since I've often found this frowned upon by others I sometimes compromise this:

IsEligibleForDiscount = (widget.Price < 10.0m && widget.UnitsInStock > 100);

// ...

It totally disregards encapsulation for the sake of cohesion but it does make the code more clear.

Also to TweZz... cool testing tool, but you're refactoring needs work. The following is much faster in firefox 3.6 and slightly faster in ie8 (though the tool seemed buggy in ie8):

Gravatar J.P. Hamilton Thursday, January 27, 2011
"How do you know it won't ever be reused? As the customer adds more functionality and enhancements?"

You don't know and you don't have to know. If and when that happens you can easily refactor. It's confusing to have objects that have one-off properties like this all over the place. It's probably a violation of the Interface Segregation Principle as well.
Gravatar Dave Herren Thursday, January 27, 2011
Never mind... my code had an error in it apologies, but I've corrected it and things slowed down dramatically. I guess I shouldn't be trying to get out a quick response at work. So you're right TweZz, very interesting results.
Gravatar Chuck Bryan Saturday, January 29, 2011
Personally, I like refactorings like this. I find that the code is much more readable.
Comments are closed.