Don’t Let Cleaning-Up Go Overboard

When a tweet pointed me to Radoslaw Sadowski’s „Learn how to make a good code by bad example“ I was excited to see how the author would go about showing how to clean-up brownfield code. Although his example looked simplistic, it turned out to be quite interesting. It’s about some price discount calculation:

I liked how the author pointed out deficiencies and started removing them through simple refactorings.

This went well for the first half of his article – but then turned really bad. To my taste he went overboard following the „Thou shalt apply all design patterns thou knowest“ commandment. Sad, very sad.

Just compare the number of lines of code:

  • Original brownfield solution: 25 lines of code (LOC)
  • Reasonably refactored solution: 55 LOC
  • Final solution: 118 LOC

By this the author says, that roughly 5 (!) times the size of the original dirty code is the pinnacle of cleanliness. Is he serious? And he even does this in the name of the DRY principle (see STEP VII in the article) and contrary to his insight „We should write as short and simple code as it is possible. Shorter code = less possible bugs, shorter time of understanding the business logic.“ (see STEP VIII).

What has gone wrong in the Clean Code camp?

A Different Approach

But I don’t want to just bemoan this well meant attempt gone overboard. Let me try to improve upon it.

Test First

As much as I liked the first refactorings of the author, I disagree that’s where you should start when cleaning up dirty code.

The first thing you always should do: Put the code to refactor under test. If there aren’t any automated tests yet, set up at least a couple of integration tests. That’s what I did when I took my stab at this refactoring problem. Here’s the tests I came up with:

I looked at the different paths through the code and tried to find simple test cases for them. They reflect how the code currently works – whether that’s ultimately correct or not.

After that all refactoring had to produce code which complied with these formalized requirements.

Interestingly the author’s final version does not pass this test. It fails on type 1 accounts („not registered“) to which it incorrectly applies a loyality discount.

How come? The author did not realize the change in the semantics of the code during refactoring, when he moved application of the loyality discount to the end of the function.

He still gets it right in an intermediate version, though:

The highlighted lines show: a loyality discount only gets applied for registered customers within the switch statement. But later the discount gets applied in all cases:

Design Before Refactor

A test could have caught this error. But how to avoid it in the first place during clean-up/refactoring? I suggest you do an explicit design first.

When I say „design“ you don’t need to fear a full blown mainstream OO design with UML diagrams. No, it’s much, much simpler. The author unwittingly almost did it, when he wrote:

Yes, an ordered list like this can be a poor man’s design to start with.

Design is about responsibilities which get translated into modules (i.e. functions and classes among others). And it’s about how those responsibilties should be put together to form the overall responsibility. The requirements state a certain behavior which then is produced through collaboration of many parts.

Despite his „design attempt“, though, I don’t agree with the author’s solution approach.

There is no calculation algorithm to be determined. And the order of processing steps is wrong – especially if it’s supposed to mean the loyality discount is always applied.

Let’s look at the dirty code and highlight the different responsibilities:

I can spot three aspects of which two are badly intertwined:

  1. The loyality discount is determined.
  2. The status discount is determined. Only registered users are eligible to status discounts.
  3. Status discount and loyality discount are applied only if the customer is eligible for a status discount.

Those aspects need to be retained – but brought in a proper order and cleaned-up. On my way to reach that goal I find it very useful to step back from hard to overlook imperative code. Instead I like to visualize the solution first in a declarative way.

Here’s a simple data flow sketching how the desired behavior can be achieved by a small process of focused steps. The colors of the functional units match the aspects found in the original code:

As you clearly can see, a loyality discount now only needs to be determined if the customer if eligible for a (status) discount. Likewise only then a discounted price is calculated.

Each functional unit now has only a single responsibility. And on top there is another functional unit to integrate them all into a whole – which also is a responsibility of its own.

Implement Clean Code Without Frills

Such a straightforward easy to understand design sure is easy to implement. Every functional unit („bubble“) becomes a function of its own:

That’s 52 lines of code, i.e. roughly twice as much code as the original. But these 52 lines of code are, well, easy to read and follow the SRP:

  • Calc_discounted_price(): Calculating a discounted price is two lines of trivial math. It’s crystal clear how the discounts are applied. As you can see, the status discount is applied first, which means status is more important than loyality (because it’s calculated based on a larger value). In Radoslaw’s final code there is no function where this can be seen this clearly.
  • Determine_discount_for_loyality(): This differs not much from the original. But giving it its own function adds meaning to the one statement and wraps a contract around the responsibility. In case the calculation changes it’s easy to spot where to intervene.
  • Determine_status_discount(): A very, very simple switch-statement suffices to map a status to a discount percentage. Here’s where Radoslaw’s solution slips into textbook overengineering. What a waste to set up interfaces and factory and strategy classes for a simple mapping! Even a dictionary data structure would do – were there not the special case of the unregistered customer. So what happens if a status discount changes? Alter the percentage in the method. What happens if a new status is introduced? Add it to the enum and add a case in the switch; that’s probably 3 lines of code. Very, very easy and straightforward.
  • Adjust(): This method’s responsibility is just to wire-up the data flow. It deliberately does not contain any logic. That’s why it probably looks a bit funny to you: no if-then-else in there. Instead the two branches of the data flow are implemented using continuations. This is to follow the Integration Operation Segregation Principle (IOSP). This is how I „mechanically“ translate a design like above; but if you don’t want to be that rigorous you could use an if-then-else like so:

In the name of Clean Code we could now argue about whether Determine_status_discount() really has only a single responsibility, though. Because currently it not only determines the status discount, but also whether any discount should be applied at all (which is different from a 0% status discount).

But, well… to me that’s only a bit of dust left in the code. Negligible dirt 😉 Not extracting this responsibility keeps the code shorter without sacrificing too much readability/evolvability.

If you like, though, extract it. In any case that would be much more reasonable than introducing design patterns to blow up the code 5 times thereby obscuring the purpose of it: to calculate a discount.

Summary

It’s great to explain Clean Code development by actually cleaning up dirty code. But as Radoslaw’s code shows me it’s very important to be careful to not overdo it. Applying all those pure principles and shiny design patterns is not an end in itself. We’ve to keep the reader in mind and stay humble.

Simplicity in coding is not about writing code quickly, but making it easy to understand.

And Clean Code is not about showing off yet another hyped concept, but staying true to what the code is supposed to do in the first place.

To that I find it useful to progress in a systematic way:

  1. Put the code to refactor under test.
  2. Identify the responsibilities in the code. Use color markers.
  3. Design a solution before any code showing how the responsibilities collaborate towards the overall required behavior. This design should be declarative!
  4. Refactor the code to your design; partially rewrite if needed.

Happy cleaning! 🙂

Posted in English postings and tagged .

7 Comments

  1. Thanks for your version of cleaning up the example.
    When I read the original version, I asked myself ‚is he serious?‘ on the step IX by introducing the strategie and factory pattern. But I should have be warned as he wanted to make the step from clean code to super clean code…

  2. The original solution can be improved just by using some simple math and naming. Here is another version, which borrows your naming, but preserves the original procedural design.

    public class Discounting
    {
    public decimal Adjust(decimal price, int accountStatus, int yearsLoyal)
    {
    if (accountStatus == 1) return price;
    var loyalityFactor = LoyalityFactor(yearsLoyal);
    var statusFactor = StatusFactor(accountStatus);
    var totalFactor = TotalFactor(loyalityFactor, statusFactor);
    return totalFactor * price;
    }

    private static decimal LoyalityFactor(int yearsLoyal)
    {
    return (yearsLoyal > 5 ? 5 : yearsLoyal) / 100m;
    }

    private static decimal StatusFactor(int accountStatus)
    {
    switch (accountStatus)
    {
    case 2: return 0.9m;
    case 3: return 0.7m;
    case 4: return 0.5m;
    default: throw new NotImplementedException(„unsupported type: “ + accountStatus);
    }
    }

    private static decimal TotalFactor(decimal loyalityFactor, decimal statusFactor)
    {
    return (1 – loyalityFactor) * statusFactor;
    }
    }

    PS: I don’t know if this is correct, because I couldn’t run your tests in MSTest and spent just a couple of minutes refactoring 🙂

  3. @Rusi: From some point on refactoring of course becomes a matter of taste. But in this case I’d say there’s still something missing:

    * No symbolic names for the account statuses. Adding those would bring your solution to some 40 lines of code.
    * More importantly, though, you’re tearing apart checking the account status: some happens in Adjust(), some in StatusFactor(). The meaning of that is not clear to the casual observer, I’d say.
    * Calculation of the discounted price is happening with some ominous totalFactor. Why is that introduced? Even though correctness is preserved (I checked it for you 😉 ) it becomes less clear how discounts are applied.

    Bottom line: I agree with you that no fancy additional classes/interfaces are necessary. The cleaned-up code can stay „procedural“ or functional. But to my taste some more effort should be invested for better understanding.

  4. The interesting thing with start with verbs vs. nouns when programming is the nature of verbs themselves. Think of philosophy, all philosophical questions start with; „What is love/life/[insert any other noun here]“. Compare this to „What does it mean to live/love/[other verb here]“. Actually, when you type „what is life“ vs. „what does it mean to live“, in the first case, you’ll get essays about life that touch on 94343 different sub-topics, while in the second case you’ll get very concrete results like „What does it mean to live a healthy life“, „what does it mean to live with integrity“?

    My point was, when you start with verbs, you are INCLINED to go to more concrete things. You are more likely to question the verb’s abstractedness and dig further for concrete meaning.

    While with nouns, it’s the opposite. When you analyze a noun, you’re likely to have with 5443 different definitions of it. And it’s easy to rationalize that all these things mean the same noun!!!

    I can see this principle with everyday programming, naming things like „Discount Manager“ vs. „Manage Discount“, for example, or „Determine discount“ vs. „Discount determiner“ (see, the second case will probably nudge people to insert more things than necessary in the „discount determiner“ class/whatever).

    That’s why, looking from a single responsibility principle, it’s probably always best to start with the nouns. Ralf provides a lot of other good reasons for this, and I think this is a good additional one…how humans are wired.

  5. Darko, this is a very interesting perspective on this.

    I guess you’re right: In order to act I have to be very specific. I can do only one thing at a time. Say this or not this, but not both. Take that or something different, but not both. Actions require „final“ decisions for a given point in time.

    But being some thing is different. Things can be many things for many people at the same time. There can be much discussion about what the are. It’s in the eye of the beholder.

    Which means: looking at the world (or at programming problems) is fundamentally different if your focus is on things/objects/nouns or actions/behavior/verbs.

    Sure the world is made up of both: actions handle things, objects and verbs both go hand in hand in a sentence. They are yin and yang. But still… where do you start with your understanding of a problem or the design of a solution?

    As you’ve pointed out: my solution starts with verbs, actions, behavior, process. Because that’s what the customer wants in the first place. And because in the end that’s where we need to be crystal clear as to what we deliver. So clear and specific it can even be automatically tested.

    Is this how we’re wired? I don’t know. Maybe wiring is depending on culture: the west vs the east for example. Nevertheless to me it seems what’s required from us (to attack most problems at least).

  6. Hello Ralf,

    I remember a german blog post of yours from 2014 (http://blog.ralfw.de/2014/06/klein-ist-okonomisch.html) where you mentioned the interesting fact that the effort to extend existing programs with new features correlates to the number of LOC. The shorter the source code, the better was the evolvability. But even when doing reasonable clean-up of code, as shown here in your example, it tends to increase the LOC, often by a factor 2 or more. Even Bob Martin’s book is full of similar example (there it os often a factor 3 or more). So if we combine this observations, someone could easily conclude „clean code is not economical“. Actually, my gut tells me that cannot be true. Do you have any idea where this reasoning is faulty?

    Thanks

    – Michael –

    • @Michael: This is a common effect: Cleaning up increases the code size. The reason is very simple: Explicitly assigning meaning needs space. It starts with meaningful names: A variable name r carries less meaning than e.g. revenue. Then you need to extract methods to add meaning to several lines of logic. This adds at least 4-6 lines in the code file. The same is true for extracted classes and assemblies etc.

      But then, fortunately, there’s also the reverse effect. Cleaning up decreases code size. More often than it increases it, I’d say. This happens if you get rid of overengineering of any kind, e.g. patterns where they are not needed, optimizations which are not needed etc.

      Also I’d say we have to interpret the finding of the study correctly: My guess is, the conclusion should not be to just try to write as few lines of code (or statements) as possible, but to define meaningful horizons of code (modules) which are small. This requires loose coupling (black boxes) of parts which have high internal cohesion.

      Take a single app with 1 million LOC. Without proper modularization (which means much more than lots of classes) that’s the horizon within which you have to search for bugs or places to enhance the code. That’s not fun.

      But then take a software system of 1.1 million LOC which does the same – but is split up into 4 applications of very distinct functionality. And each of those apps consists of 10 loosely coupled components with very specific responsibilities. That’s 4×10=40 components with an even narrower horizon within which to work.

      So in fact we’re not talking about 1.1 mio LOC, but 275000 LOC for each app or some 28000 LOC for each component. That’s much less relevant lines of code with regard to each requirement which very likely pertains to only one app or even just a single component.

      Even though the total LOC count is higher there is more evolvability in the second scenario. Because the additional LOC went into really useful compartmentalization of the logic. (Still, though, I assume as clean up version of the first app would consist of much less code.)

      Hope this supports you with your gut feeling 😉

Comments are closed.