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
app_from_dot_ninefold_file. - Easy to understand what the method does.
Step 1
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.
Step 2
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:
To this:
Step 3
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:
Step 4
I notice that yield is called in 3 different places and decide to DRY it up.
Step 5
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.
Happy Hacking!
