Opening a window on codesmells 💩💨
Tags: ruby, refactoring, codequality, beginners
Published: 2020-09-15 07:46:00

[Codesmell](https://martinfowler.com/bliki/CodeSmell.html) 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](https://sandimetz.com/), 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](https://youtu.be/PJjHfa5yxlU)
* [All the little things, Rails Conf 2014](https://www.youtube.com/watch?v=8bZh5LMaSmE)

## 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 blame`ing - 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 `if`s, `elsif`s and `else`s
* 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.

```ruby
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 👉🚪.

```ruby
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.
```ruby
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:

```ruby
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.