Get Rid of That Code Smell – Control Couple

Posted by on April 11, 2012 Leave a comment (19) Go to comments

This is a post from the Get Rid of That Code Smell series.

If you are serious about Object Oriented Design and respecting Single Responsibility Principle then you definitely want to get rid of Control Couple code smells. In this post I will show you a simple example explaining how to identify and remove control coupling from your code. I like to think about that code smell also in the context of SRP because I like to apply it to every piece of my system – whether it’s a method, a class or a whole library. I like to be able to describe each piece with a simple sentence saying what it does, what’s the responsibility. With control coupling you introduce multiple responsibilities in a single method which is against SRP and against Object Oriented Design.

Detecting Control Couple Smell

Reek can help you with that – detecting Control Couple is turned on by default. I believe that Pelusa’s “Else Clause” and “Case Statement” lints can also be used to find places in your code with potential control coupling.

A dead-simple example of control coupling could like this:

Which is pretty self-explanatory. The say method puts an upcased sentence if loud parameter is set to true.

Let me show you a real-world example though. Let’s bring it to the class level – here’s a small snippet from Virtus:

DefaultValue is a class responsible for evaluating a default value of an attribute in Virtus. Its #evaluate method, depending on @value ivar, is deciding how to evaluate the value. We have multiple cases here and all of them are handled within one method. You cannot easily describe this method’s responsibility because it does different things depending on what the @value ivar actually is. If it’s a proc-like object responding to #call we call it, if it’s a duplicable object then we dup it, else we just return it as is.

I found that code pretty ugly. The repercussion of Control Couple smell in this example was that every time we were setting the default value we were performing this if/else check on @value.

Removing Control Couple Smell

The DefaultValue class was refactored by splitting the evaluate logic into 3 sub-classes. Every sub-class implements a self.handle? method which checks if its instance can actually handle the given value. This means that the logic inside #evaluate is now performed only once, prior to deciding which DefaultValue sub-class we want to initialize.

Here’s how it looks like:

Now for example FromCallable class implementation looks like that:

I liked that refactor because I could remove the if/else clause from #evaluate method, split responsibilities across 3 sub-classes and gain a small performance boost too.

Summing Up

Even if removing Control Couple smell requires writing a bit more code – it’s worth that price. Getting rid of that smell leads to better Object Oriented Design and helps you with respecting Single Responsibility Principle. I also like that it’s easier to understand what the code does because responsibilities are shared across the objects rather than jamming everything into a few methods that couple the logic to the received arguments.

In the next post we’ll see how to deal with Duplication in our code. Stay tuned.

Blog, , , ,
  • Saxon

    Hey, sorry if this sounds dumb, but what does def evaluate(*) do? As best as I can guess and tell in IRB, it can take in any number of parameters but ignores them all. Is this correct?

    • http://solnic.eu/ solnic

      Correct. It basically means that a method can accept any number of args, including no args at all.

    • http://henrik.nyh.se Henrik N

      Yes, it’s like def foo(*args) which allows any number of args and makes an array of them in the args local variable, but without the variable.

  • Jedwabisty

    i know that this is trivial example, but base class knowing about descendants is also antipattern

    • http://henrik.nyh.se Henrik N

      Do you have a better idea for an example such as this? You have to capture the mapping from condition to class somewhere, and this seems a nicer solution than many.

      I’ve done similar things where the build method would do a case/when on the input to select the class. Seems a bit more maintainable to only have to list classes, and let each class be responsible for the conditions.

      It would perhaps be nicer still if each subclass would register with the superclass as they were loaded, or if the superclass could find out its subclasses dynamically, but that kind of thing would be tricky with Ruby on Rails lazy loading.

      • http://solnic.eu/ solnic

        Actually the code examples are not 1:1 to what we have in Virtus where we do have a descendants tracking mechanism so it’s all dynamic and there is no DESCENDANTS constant. I used it here just to simplify the example. Thanks for pointing that out.

        • http://henrik.nyh.se Henrik N

          I’m looking at https://github.com/solnic/virtus/blob/master/lib/virtus/support/descendants_tracker.rb. How does that work with Rails lazy loading? I’m under the impression that this would happen:

          MySuper.descendants  # => []
          MySub  # This class is loaded only now on first mention.
          MySuper.descendants  # => [MySub]

          But perhaps Rails loads all models up front these days.

          • http://henrik.nyh.se Henrik N

            We just tried applying something like this to some real code and we did indeed have problems with Rails lazy loading.

            We ended up explicitly requiring the subclasses in the superclass, which is far from ideal. One improvement would be to not mention each subclass by name when requiring, but instead matching filenames/directories. Still not too pretty, but that would get you to the point where the superclass barely knows anything about the subclasses, and you can support an additional subclass just by adding it.

          • http://solnic.eu/ solnic

            Yes this approach won’t work in rails due to lazy loading of files. You need to handle it yourself one way or another. You can explicitly require all the files and that’s what I usually do in rails.

      • Tony Collen

        I’ve done something like allowing the class to be configured from the outside to know about which handlers to use. Then you can plop something in an initializer (in the Rails world, for example) and add a new handler to the list when you need to. 

        It’s not too bad, in adding moving pieces, and doesn’t necessarily force you to subclass handler classes if you don’t want to.

    • Pieter

      Interesting… @solnic, what are your thoughts about this?

    • http://solnic.eu/ solnic

      Agreed although in simple cases like that I think it’s simply a pragmatic choice to implement a factory method like DefaultValue.build. If things become messy you can always add higher-level factory-like classes but that’s IMO a heavy artillery and you usually just don’t need that.

  • Kdjtar

    This solution reminds me a lot state/strategy pattern.

  • DmitriyNesteryuk

    I like this approach. Cool!!! Recently, I read about the same approach in Avdi’s book there http://objectsonrails.com/#sec-12-5, unfortunately, I forgot about that. Thank you, you reminded me and again emphasized importance of that.

  • http://kresimirbojcic.com Kresimir Bojcic

    I see you’ve changed the code to use DescendantsTracker to figure out descendants instead of hardcoding… Solution is really elegant. My only worry is that now the exact order is lost. (Does callable wins over dup? I am not sure).

    In that first snippet I’ve found the abstracion level mismatch confusing. One line it is method call, the other you have low level instance variable.

    When I’ve fixed that, it got me in a different direction:

    class DefaultValue
      def initialize(value)
        @value = value
      end
      def evaluate(instance)
        if @value.respond_to? :call
          @value.call instance
        elsif @value.respond_to? :dup
          @value.dup
        else
          @value
        end
      end
    end

    Now that it’s all same level I went for this:

    module Kernel
      def __self__
        self
      end
    end

    class DefaultValue
      def initialize(value)
        @value = value
      end

      RESPONSE_ORDER = [ :call, :dup ]

      def evaluate(instance)
        @value.method(message).arity == 0 ? @value.send message : @value.send message, instance
      end

      def message
        @message ||= (RESPONSE_ORDER << :__self__).select { |msg| @value.respond_to? msg }[0]
      end
    end

    This is untested, probably not working, and possibly stupid because I don't know the background. But I feel it might be good enough abstraction for the case you've showed. It might be a little less readable. From readability standpoint the 'if' version is king anyways. I feel this would be a little easier to extend (when you know how it works) but a little less flexible than your solution.

    All in all if the only use is for @value to respond in a prefered way it might be good enough.

  • Sven Svensson

    Shit, this was an ugly refactoring. #fail

  • Beau Fabry

    Does anyone else feel like though this code obeys SRP better, it’s much more *complicated* and much harder to read or understand than the original?

    • TheWayOfTheGun

      Absolutely. I could read and absorb the first version immediately. The second required a bit of examination. Isn’t readable, comprehensible code the ultimate goal? That refactoring looks to me like the principle was followed for its own sake. SRP is great, but like any other principle we must keep our eyes open and decide when to adhere to the principle and when not to.

      • http://solnic.eu/ solnic

        The refactored code is actually *simpler* than the original. Yes, it is a bit harder to understand as now you have more pieces that you need to learn about. This is not an argument for me though. The simplicity of the refactored code comes from the fact that you have separate objects handling all the cases. When you ask me how a callable value is set I’m not going to say “there’s this #evaluate method which performs a check on the value and if it’s callable it calls it” instead I’ll say there’s a DefaultValue::FromCallable instance which is responsible for that. This is a significant difference.

        It’s often the case when showing a refactor towards better OO design that people complain that resulting code is harder to understand. Benefits might not be so obvious when you show a single example. Smells become problematic when your code base grows. The ultimate goal is to have code that’s easy to change. Even when wrapping your head around everything is harder at the end, it’s still worth that price because once you understand it you will be able to change it more easily.

        I guess the reason why OO design is harder to understand is because there are more abstractions that you need to learn about. In my original code I had a single method with an if statement. After refactor I have 3 separate classes and a factory method finding the best class to handle a given value – it’s much more to learn I agree. I’ll repeat though – that’s not an argument at all for me, making code easier to change (and faster in this case too) is more important than understanding it a bit quicker.