Opening a window on codesmells 💩💨

Codesmell sure is a great word, it was first introduced by Kent Beck in the 1990s and later popularised in the book Refactoring: Improving the Design of Existing Code written by Martin Fowler in 1999. Just like the name suggests it's a pong or stank that emanates from code. Suffice to say, the code in question is less than ideal.

Over a series of small posts I'm going to a uncover a few of the easier to identify codesmells and give you some tools to fix them.

Sandi Metz, my favorite technical speaker and writer, has written and spoken about these topics extensively. I thoroughly recommend checking these resources:

• Get a whiff of this, Rails Conf 2016
• All the little things, Rails Conf 2014

Trust your gut

❌ test fails: 🤔 "why isn't this working?"
✅ test passes: 🤔 "why is this working?"

Familiar scenario? As a complete n00b literally everything was baffling. I considered it a huge win if I even began to understand the code I was reading. If things were tough, I'd hear a little voice:

"You've read over this class or method a million times, why can't you understand it? Adding a new spec shouldn't be that difficult, what's wrong with you?".

What if it wasn't you but it was the code (don't bother git blameing - it was probably you last month 😅)? Complex code isn't good code. Learning to trust your intuition only comes with confidence but it's important to remember that if something doesn't feel right, often it isn't!

Any fool can write code that a computer can understand, good programmers write code that every human can understand.
-- Martin Fowler

Start off small 👀

A couple of relatively easy to spot issues:

• Conditional complexity in the form of way too many ifs, elsifs and elses
• The length of Classes, Modules and Methods

Let's take a look at these things in a little more detail.

Conditional complexity

You're working on adding a small feature or fix and need to add a simple expectation or two to an existing unit spec to cover this case. How hard can that be?

Upon opening that my_thing_spec.rb file (because you practice TDD of course 😉) you discover that can be pretty hard. The spec is overly complex, the set up alone fills your screen, and scrolling down the file your your enthusiasm starts to wane.

A tonne of nested contexts is most likely a hint that something isn't quite right.

describe '#my_understanding'
 context 'when i see this'
  context 'when it looks like this'
  context 'when it looks like that'
  context 'when it looks like something unrelated'
  context 'when it arrrghhhhhhhh'

If your unit spec feels difficult to work with, it's highly likely you're looking at an opportunity to banish a codesmell 👉🚪.

describe '#error_message' do

   context "with an 'error' response" do
     it 'has an error describing the error' do

     end
   end

   context "with a 'refused' response" do
    end  

    context "without a response present" do
      context "and a status code" do
         ...
       end
     end

     context "and no status code" do
       ...
     end
   end
  end
end

In the above (semi-fabricated and overly simplified) example we're testing an error_message method that clearly has a few different return values depending on the context.

Looking at the corresponding method that we're testing, it's no surprise there are a few conditionals.

def error_message
  if error_response?
    "#{response["errorCode"]} #{response["errorType"]}: #{response["message"]}"
  elsif payment_result == 'refused'
    "Payment refused: #{response["refusalReason"]}"
  elsif status_code.present?
    "#{status_code} status received"
  else
    "Payment connection failed"
  end
end

In fact we've got all the if, else and elsif bases covered and more.

Asking myself - What does the "error_message" method do?

Wellllll if it's an error response it builds an error message and if the request was refused we bubble that reason up and if we don't get an error or a reason but there's a status code we put that in the error and if all else fails, we just state that the request failed.

• 11 lines of code
• 3 ands.

What would Sandi Metz do? I do know what she'd says:
Methods should be no longer than 5 lines of code.

Now I'll admit, I do break that rule (maybe a little more than sometimes). But the and rule is a good one to stick to. If you have to say AND when describing it's function it's doing too much! So, we've identified a conditional complexity codesmell and an opportunity for a refactor.

Zooming out to the class level now:

module Clients
  class PaymentResult
  attr_reader :result, :api_version

    def initialize(result:, api_version:)
      @result = result
      @api_version = api_version
    end

    def response
      result[:response]
    end

    def payment_result
     response["resultCode"]&.downcase unless response.nil?
    end     

   def success?
    status == 200 && response == 'authorised'
   end

  def failure?
    status == 200 && ['error', 'refused'].include?(result)
  end

  def error_response?
   response.has_key?("errorCode") unless response.nil?
  end       

  def error_message
    # you've already seen me :)
  end

  def insufficient_funds?
    response["refusalReasonCode"] == "5"
  end

  def card_expired?
    response["refusalReasonCode"] == "2"
  end

 def declined?
   ["7", "9"].include?(response["refusalReasonCode"])
  end

 end
end

We can ask ourselves a similar question –– What is this class doing?

Wellll, it handles/parses the response from the external third party payment provider AND if there are errors it then formats the error according to the type of resultMessage received"

It's a relatively small class (doesn't exceed Sandi's 100 line limit for Classes rule). There is another codesmell lurking in there: primative obsession. But I'll park that one for another post (or justification) soon. There was an and in our class explanation – and a sign that the class is doing more than it needs to.

We have green tests and our existing code works so that means we can refactor with confidence 🙌

Just a simple refactor?

Pragmatism and discretion are always an essential ingredients in our work as Software Engineers. There are never absolutes, only trade offs that can be lived with or not. Refactoring isn't always the answer but it usually is. Investing more time than is necessary isn't always called for, it's up for to you to decide what's appropriate for the given circumstances.

Ask yourself a couple of things before embarking:

Is this code likely to ever need to change?

If it's unlikely to be revisited it's okay to leave it. Honestly, bad code isn't necessarily bad if it was cheap and is robust and does it's thing without needing to change. You don't get paid to be perfect. You get paid to build the right tool for the job at the right time.

Is a refactor going to help the next person who will inevitably need to touch this code?

If the answer is hell yeah - then why not leave things in a better state than what they were when you arrived. Investing in clean code can take time but saves it in the long run.