Today I’m going to review and refactor some code from Ninefold CLI. I picked a method I could make better. Ninefold is a hosting company and
pick_app is apparently responsible for selecting an app for CLI to work with.
What’s good about this code:
- Long method names like
- Easy to understand what the method does.
On line 4, if app was specified in a dotfile or environment variable, we use that app. I think that there’s no need to go into detail here about how exactly app was specified, so I extract method
user_specified_app and end up with this:
It is a bit easier to read now.
pick_app method uses
block.call everywhere, instead of
yield for no obvious reason.
yield is better in my opinion because:
- It’s one word instead of two.
- You get syntax highlighting for it.
- It’s a standard Ruby way of calling passed block, so it adheres to the Rule of Least Surprise.
So it goes from this:
At this point, lines 9-17 are still too complex. It’s possible to understand what’s going on, but it takes more effort than it should. So, I extract two more methods:
I notice that
yield is called in 3 different places and decide to DRY it up.
apps.count > 1 isn’t particularly easy to read and can be replaced by
app.many? from Rails’ ActiveSupport.
apps.count == 1 can be replaced by
apps.one?, but it’s not so straightforward as
many? because it returns number of Enumerable elements that are truthy, not the same semantics as size of 1.
For further reading, the pull request conversation with a nice person from Ninefold.