The today I was writing some code and it just felt weird when I started to write this long conditional. I asked a coworker of mine did he have any ideas to what we could do to make this more readable. We did the following
Before
class GetStuff
def fetch(current_url)
response = handle_response(query).try(:first)
return nil if !enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url
response
end
private
def enabled?(value)
return false if value.nil?
value.enabled
end
def postable?(value)
Time.at(value.post_date) < Time.now && value.enabled
end
end
After
class GetStuff
def fetch(current_url)
@current_url = current_url
@response = handle_response(query).try(:first)
return nil unless show?
@response
end
private
def show?
return false if @response.nil?
enabled? && postable? && is_current_page?
end
def is_current_page?
false if @current_url == @response.global_banner.first.button_url
end
def enabled?
@response.enabled
end
def postable?
Time.at(@response.post_date) < Time.now && @response.enabled
end
end
We did a couple of things here we removed some reverse logic here.
!enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url
We don't need the ! anymore. I don't know about you but the less I have to think about if this thing is true reverse it the better.
We also simplified the variables being passed through to methods by making some instance variables.
Before
def fetch(current_url)
response = handle_response(query).try(:first)
return nil if !enabled?(response) && !postable?(response) && current_url ==
response.global_banner.first.button_url
response
end
After
def fetch(current_url)
@current_url = current_url
@response = handle_response(query).try(:first)
return nil unless show?
@response
end
We have a gaurd clause that is protecting us against a empty response from a server. We already had this but it wasn't really clear what it was doing and that it was protecting everything.
Before
return nil if !enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url
def enabled?(value)
return false if value.nil?
value.enabled
end
After
def show?
return false if @response.nil?
enabled? && postable? && is_current_page?
end
Things are more readable and the methods explain why we have the checks in place and what is this condional doing. I feel like this is self documenting code.
Top comments (3)
Hey,
cleaning up code is always a good idea! Long conditional chains are indeed horrible to read and cost a lot of brainpower to process. Some thoughts on your refactoring:
1)
is the same as
if you prefer it a bit shorter.
2)
return nil unless show?
is the same asreturn unless show?
, there is no need to explicitly state thenil
, a blankreturn
always returnsnil
3)
handle_response(query).try(:first)
will only work if you use the rails gem, becausetry
is from rails. Instead, you can use the Ruby safe operator&
:handle_response(query)&.first
which removes your dependency on rails for this service.4) would
be the same as
but just without one method call (
if
) less? Also, this method now will returnnil
if yourif
statement returns false, which seems strange for a method ending in a?
, because for these methods I expect a boolean response.5) The naming seems inconsistent, you have
enabled?
but thenis_current_page?
- so I would either change your other methods to also start withis_
or remove theis_
. I personally prefer to not start method names withis_
since it is not really necessary for readability in my opinion (English is not my native language, so my feeling for the english language might be wrong).Thank you for taking time to comment and share your knowledge I really appreciate it! :)
You're welcome :-)