Please Don't Write Clever Code

I came across this code the other day, the author was quite proud of it but I would reject this PR in a heartbeat. I’m not trying to disparage the author, most of us have gone through a “clever” code phase, myself included. I do want to take the time to point out why I think this code is bad.

class LogController
  def initialize(progname = nil)
    @progname = progname
    @supported_storage = Setting.supported_log_storage&.split(',')
    storage = Setting.active_log_storage&.upcase
    setup_driver_for(storage)
    @active_storage = storage
  end

  def process(input, storage = nil, other_storage = nil)
    storage ||= check_active_storage
    type = prefix_from(storage)

    klass = Object.const_get("#{type}Log")
    log = klass.new(input)
    log.driver = setup_driver_for(storage)
    return true if log.save

    recall(:process, input, storage, other_storage)
  end

  def where(query, storage = nil, other_storage = nil)
    storage ||= check_active_storage
    type = prefix_from(storage)

    klass = Object.const_get("#{type}Log")
    results = klass.where(query)
    return results unless results&.empty?

    recall(:where, query, storage, other_storage)
  end

  def recall(method, data, storage, other_storage)
    other_storage ||= (@supported_storage - [storage])
    storage = other_storage.pop
    unless storage
      return [] if method == :where
      raise LogControllerError.new(log: data) if method == :process
    end

    send(method, data, storage, other_storage)
  end

  private

  def check_active_storage
    @active_storage = Setting.active_log_storage&.upcase
  end

  def prefix_from(storage)
    return 'Es'  if storage == 'ELASTICSEARCH'
    return 'Pg'  if storage == 'POSTGRES'
    return 'Sql' if storage == 'SQLITE'
  end

  def setup_driver_for(storage)
    return @driver if storage == @active_storage

    LogDriver.new(
      type: storage,
      progname: @progname,
      host: ENV['LOG_DRIVER_HOST'],
      port: ENV['LOG_DRIVER_PORT']
    )
  end
end

The reason I’m putting quotes around clever here is because the code itself is not clever. Actual clever code solves the problem in a novel obscure way that that often simplifies the code. For example the classic fast inverse square root is clever code. “Clever” code on the other hand reads as a laundry list of obscure language features that are used for no apparent reason other than to signal how well the author knows the language.

The code above is a prime example of “clever” code. There’s tons of metaprogramming and pointless recursion which add no value and serve only to validate the authors ego and obfuscate what the code is actually doing. Most of the code in this example only exists to support the baroque choice of implementation. If you take the time to decipher the code above you’ll find out that LogController is an abstraction to wrap around multiple logging backends. You can configure the order in which these backends will be queried in the settings of the application. This is a pretty simple problem and definitely does not require metaprogramming or recursion.

Below I’ve rewritten the code the “dumb” way which requires no metaprogramming or recursion. While it may not be as “cool” and doesn’t show off my knowledge of ruby it reads well and is about three times shorter.

class LogController
  def initialize(progname = nil)
    logger_backends = {
      'ELASTICSEARCH' => EsLog.new(progname),
      'POSTGRES'      => PgLog.new(progname),
      'SQLITE'        => SqlLog.new(progname),
    }
    @loggers = Settings.log_backend_priorities.split(',').map { |k| logger_backends[k] }
  end

  def where(query)
    @loggers.each do |logger|
      results = logger.where(query)
      return results if results.present?
    end
  end

  def process(log)
    @loggers.each do |logger|
      return if logger.process(log)
    end
  end
end

Please don’t write “clever” code because the “dumb” code is usually better in every way.

Please Don’t Write Clever Code

or: Your Code is Baroque-n