Sep 28 2013

Keep Your Code Simple, Stupid

Editor’s note: This post first appeared in a slightly different  format on Jonathan’s personal blog.

Simplicity concerns concepts and divisions more that it does lines of code. But you probably didn’t hear that from Kelly Johnson, who coined this phrase:

Keep it simple, stupid.

The principle emphasizes that, contrary to what one might presume, complication isn’t a sign of brilliance; it signals a lack of investment. It shows the inability to break down and sift through the aspects of a problem domain to arrive at a solution that could be explained to a coworker in a couple of sentences.

The sad reality is that no tool can assess (or automate) simplicity; CodeClimate can’t tell me how well my knowledge boundaries are defined. Most of the time it does an impressive job at guessing through the causal relationship: a simpler model often yields simpler code.

But not always. That’s what you as a developer are employed to do: to keep it truly simple.

Canned Simplicity

As the debate has moved away from the general principles of simplicity, we take them for granted. We perform code reviews and raise eyebrows (as we ought) when confronted with a mass of unreadable code and cryptic documentation.

But the intuition behind simplicity often conflicts with the bystanders’ definition.

For your consideration…

Before the addition of ActiveSupport::Concern in Rails 4, many of us would have coded “behaviors” by hand in the lib directory. StatusableSortable, … most with trivial functionality that simply added an order scope or a few convenience methods. Consider a SearchMixin for adding basic search functionality across several fields:

    module SearchMixin
      def self.included(mod)
        mod.extend ClassMethods
      end

      module ClassMethods
        def searchable(*args)
          @searchable = true
          columns = args
          search_string = columns.map {|c| "(#{c} LIKE :s)" }.join(" OR ")

          class_eval <<-"END"
            scope :search, lambda {|string| where('#{search_string}', :s => "%\#{string}%") }
            def self.searchable?
              @searchable ||= false
              @searchable
            end
          END
        end
      end
    end

    class ActiveRecord::Base
      include SearchMixin
    end

This mass of class evals effectively reduces a search scope declaration on a model from:

    class Product < ActiveRecord::Base
      scope :search,
        lambda { |string| where(
          'title LIKE :s OR description LIKE :s',
          :s => "%#{string}%"
        )}
    end

Down to the more aesthetically pleasing:

    class Product < ActiveRecord::Base
      include SearchMixin
      searchable :title, :description
    end

Three observations: first, I don’t recommend you solve this particular problem with that implementation (class_eval can be evil, and your security-conscious co-workers may shun you). Still, the resultant model code is much nicer. Finally, it took a major sacrifice: we had to code, debug and think out an entirely new interface for declaring the same resultant implementation. The trade-off isn’t as sweet as we would like.

So the search functionality is simple, but there’s enough going on in that search method that we might convince our skeptics that the extraction is necessary to keep things simple (or DRY), say in the event we need to add several more fields or apply some field name sanitization. But what about a trivial behavior, like status? (Again, you probably shouldn’t implement such a feature this way…)

    module StatusMixin
      def active?
        status.to_sym == :active
      end

      def inactive?
        status.to_sym == :inactive
      end

      def status
        super.to_s
      end

      def self.included(mod)
        mod.extend ClassMethods

        # Redefine the belongs_to method from the model's context
        mod.singleton_class.send :define_method, :belongs_to_with_unscoped do |*args, &block|
          # Call original belongs_to to setup actual association
          belongs_to *args, &block # belongs_to_without_unscoped

          # Setup unscoped method
          association = args.first
          klass = association.to_s.classify.constantize

          class_eval <<-"END"
            def #{association}_with_unscoped
              # Fetch association with default scope disabled
              #{klass}.unscoped { #{association}_without_unscoped }
            end
            alias_method_chain :#{association}, :unscoped
          END
        end
      end

      module ClassMethods

      end
    end

This example is not only longer, but does less: it really only adds a few one-liner methods for readability convenience.

Breakdown

Why trouble with Ruby metaprogramming to extract a few lines of code for a couple models? At that rate, why not extract every duplicate line of code into its own module?

For most of us, the former challenge would merit the canned response: “it’s easier to maintain and prevents duplication of knowledge.” Maybe our skeptics will swallow that reasoning in the first example, but probably not the second. It seems like we put KISS on hold for another date. Or did we?

Real Simplicity

We shouldn’t need to compromise between simplicity and ease of maintenance: in our quest for true simplicity, we should qualify what it accomplishes. What attributes of our code should it enhance?

Testing (without the pain)

Simple code should be simple to test: if behavior-driven development can’t get its fingers in the crannies, how can we expect another developer to extend or experiment with the code to understand it?

Consequently, nasty tests full of stubs and calls to #instance_variable_get are a quick indicator that the implementation architecture isn’t simple. Tests are part of your source code: if you had to take the same convoluted paths in implementing code for a model, it would trigger a refactor. The same goes for tests.

Encapsulation (set boundaries)

The simplification process should naturally encapsulate a domain of knowledge in one place. Thus, strictly domain related logic should be coded in that location, but once code becomes unruly, it is likely time to split the domain into new aspects or behaviors.

The strategy that arises from this principle keeps things DRY and tends towards the Single Responsibility Principle.

The side benefits from proper encapsulation inherently cross over into documentation.

Documentation (code > comment fences)

A 1:1 ratio of code to inline comments is baaad.

Often the goal of extraction is to explain the knowledge structure and separation of behaviors with as few comments as possible, i.e., “self documenting” code.

By separating responsibilities into files, classes, modules and bundles (e.g., gems), boundaries in code begin to correspond with similar boundaries in the knowledge domain. Furthermore, each boundary becomes an opportunity for pertinent (but succinct) documentation.

At the bundle level (say a Github repo), we can attach a name (which doesn’t say much in gem land) and README to outline responsibilities at a high level. As developers dive into the source code, the modules and classes lend a useful context for understanding the architecture—even without explicit documentation.

Revelation (of gaping holes)

Perhaps the less-obvious characteristic of simplicity is that it should point an ugly finger at chasms: if it’s hard to make something simpler, a key component or workflow (also simple) is missing.

This observation is exactly what led to the Rails framework! Trying to do hard things simply requires a bridge made entirely of simple units. Dedication to simplicity shows common needs that ought to be better handled.

Similarly, the search for stating knowledge boundaries simply leads to simplified methods. For example, ActiveSupport::Concern came out of the need to abstract class behaviors which were unrelated to the class’s main responsibility. Yet given the use-case, the implementation is incredibly straightforward (for Ruby metaprogramming at least):

    # rails/activesupport/lib/active_support/concern.rb
    module ActiveSupport
      module Concern
        def self.extended(base) #:nodoc:
          base.instance_variable_set("@_dependencies", [])
        end

        def append_features(base)
          if base.instance_variable_defined?("@_dependencies")
            base.instance_variable_get("@_dependencies") << self
            return false
          else
            return false if base < self
            @_dependencies.each { |dep| base.send(:include, dep) }
            super
            base.extend const_get("ClassMethods") if const_defined?("ClassMethods")
            base.class_eval(&@_included_block) if instance_variable_defined?("@_included_block")
          end
        end

        def included(base = nil, &block)
          if base.nil?
            @_included_block = block
          else
            super
          end
        end
      end
    end

Revisiting the Code

With a better understanding of real simplicity, behavioral modules likeSearchMixin have become our alibi in the pursuit of clean design.

Thanks to the principle of testing simplicity, we can unit test our SearchMixinfunctionality to make sure SQL injection attacks are mitigated.

Because of encapsulation, our code (which might have vulnerabilities) is in one logical place, is kept DRY, and succinctly defines the knowledge boundary of what it means for a model to be “searchable.”

With encapsulation documenting boundaries and keeping methods simple, we really don’t need many comments: our code is already understandable.

During the process, we realized how common it is to abstract behaviors in Ruby. Instead of tackling a few instances of the problem individually, we (or rather, the Rails core team) generalized the workflow into a reusable, unit-tested and documented feature.

That’s real simplicity. William of Ockham would approve.

No Comments

Leave a Comment

Join the discussion. Do not worry, your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>