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:
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 andelse
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.
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.
Top comments (0)