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 endAfter
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 endWe did a couple of things here we removed some reverse logic here.
!enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_urlWe 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 responseendAfter
def fetch(current_url) @current_url = current_url @response = handle_response(query).try(:first) return nil unless show? @responseendWe 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_urldef enabled?(value) return false if value.nil? value.enabledendAfter
def show? return false if @response.nil? enabled? && postable? && is_current_page?endThings 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)
- LocationEuropa
- EducationB.Sc. Mathematics and Computer Science
- Joined
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)
def fetch(current_url) @current_url = current_url response = handle_response(query).try(:first) response if show? endis the same as
def fetch(current_url) @current_url = current_url @response = handle_response(query).try(:first) return nil unless show? @response endif 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
def is_current_page? @current_url != @response.global_banner.first.button_url endbe the same as
def is_current_page? false if @current_url == @response.global_banner.first.button_url endbut 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 haveenabled? 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).
For further actions, you may consider blocking this person and/orreporting abuse
