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.
| def pick_app(&block) | |
| require_user | |
| if app = app_from_dot_ninefold_file || app_from_env_id | |
| block.call app | |
| else | |
| load_apps do |apps| | |
| if apps.count > 1 | |
| if app = interaction(:pickapp, apps) | |
| save_app_in_dot_ninefold_file(app) | |
| block.call app | |
| end | |
| elsif apps.count == 1 | |
| say "▸ You have only one app (#{apps[0].name}) proceeding...", :yellow | |
| block.call apps.first | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| end | |
| end |
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:
| if app = user_specified_app | |
| block.call app | |
| ... | |
| private | |
| def user_specified_app | |
| app_from_dot_ninefold_file || app_from_env_id | |
| end |
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:
| def pick_app(&block) | |
| require_user | |
| if app = user_specified_app | |
| block.call app | |
| else | |
| load_apps do |apps| | |
| if apps.count > 1 | |
| if app = interaction(:pickapp, apps) | |
| save_app_in_dot_ninefold_file(app) | |
| block.call app | |
| end | |
| elsif apps.count == 1 | |
| say "▸ You have only one app (#{apps[0].name}) proceeding...", :yellow | |
| block.call apps.first | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| end | |
| end |
To this:
| def pick_app | |
| require_user | |
| if app = user_specified_app | |
| yield app | |
| else | |
| load_apps do |apps| | |
| if apps.count > 1 | |
| if app = interaction(:pickapp, apps) | |
| save_app_in_dot_ninefold_file(app) | |
| yield app | |
| end | |
| elsif apps.count == 1 | |
| say "▸ You have only one app (#{apps[0].name}) proceeding...", :yellow | |
| yield apps.first | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| end | |
| end |
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:
| def pick_app(&block) | |
| require_user | |
| if app = user_specified_app | |
| yield app | |
| else | |
| load_apps do |apps| | |
| if apps.count > 1 | |
| yield app if app = ask_user_to_specify_app(apps) | |
| elsif apps.count == 1 | |
| yield auto_select_the_only_app(apps) | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| end | |
| end | |
| private | |
| def ask_user_to_specify_app(apps) | |
| if app = interaction(:pickapp, apps) | |
| save_app_in_dot_ninefold_file(app) | |
| app | |
| end | |
| end | |
| def auto_select_the_only_app(apps) | |
| say "▸ You have only one app (#{apps[0].name}) proceeding...", :yellow | |
| apps.first | |
| end |
Step 4
I notice that yield is called in 3 different places and decide to DRY it up.
| def pick_app(&block) | |
| require_user | |
| app = user_specified_app || load_apps do |apps| | |
| if apps.count > 1 | |
| ask_user_to_specify_app(apps) | |
| elsif apps.count == 1 | |
| auto_select_the_only_app(apps) | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| yield app if app | |
| end |
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.
| def pick_app(&block) | |
| require_user | |
| app = user_specified_app || load_apps do |apps| | |
| if apps.many? | |
| ask_user_to_specify_app(apps) | |
| elsif apps.one? | |
| auto_select_the_only_app(apps) | |
| else | |
| puts "You don't have any apps" | |
| end | |
| end | |
| yield app if app | |
| end |
For further reading, the pull request conversation with a nice person from Ninefold.
Happy Hacking!
