Have you seen Ruby code like this?
class AwardsController < ApplicationController def create User.find(params[:user_id]).give_award alert_user_on_slack('Congratulations!') end private def alert_user_on_slack(text) slack_client = Slack::Web::Client.new im_id = slack_client.im_open(user: @user.slack_id).fetch(:channel).fetch(:id) slack_client.chat_postMessage(channel: im_id, text: text) slack_client.chat_postMessage(channel: '#awards', text: text) end end
And here are the tests:
require 'rails_helper' describe AwardsController do context '#create' do it 'awards the correct user' do stub_slack_client post :create, user_id: 3 expect(User.find(3).awards.count).to eq(1) end it 'DMs the user' do client = stub_slack_client post :create, user_id: 3 expect(client).to have_received(:chat_postMessage).with( hash_including(text: 'Success!') ) end def stub_slack_client client = instance_double(Slack::Web::Client, chat_postMessage: nil) allow(Slack::Web::Client).to receive(:new).and_return(client) allow(client).to receive(:im_open).and_return(channel: { id: 'M123' }) client end end end
(This is adapted from a real Slack bot that I wrote to help teammates publicly award each other at Hired.)
It doesn’t look so bad at first glance, but there are a few problems that show up on closer examination:
The Wrong Abstraction
It can be hard to find the right abstraction for your code. In this case, there are a few levels of abstraction (as well as separate concerns):
* The controller code is high-level
* The Slack code is lower-level and requires the controller (and tests) to know a lot about the Slack API
Slack’s API client matches the Ruby methods (like `chat_postMessage`) to their URLs (like
[`chat.postMessage`](https://api.slack.com/methods/chat.postMessage)). But your application code doesn’t need to know about the Slack API’s URLs. It should operate at a higher level of abstraction, like “alert the current user”. To wrap that logic, create a new class with high-level methods that understands how to translate them into one or more Slack API calls.
That new class might look something like this:
# app/models/slack_client.rb class SlackClient def initialize(text) @client = Slack::Web::Client.new end def alert_user_to_award(recipient_id:, text:) message(recipient_id: '#awards', text: text) direct_message(recipient_id: recipient_id, text: text) end def message(recipient_id:, text:) @client.chat_postMessage(channel: recipient_id, text: text) end def direct_message(recipient_id:, text:) im_id = @client.im_open(user: recipient_id).fetch(:channel).fetch(:id) message(recipient_id: im_id, text: text) end end
And the controller now looks like this:
diff class AwardsController < ApplicationController def create User.find(params[:user_id]).give_award - alert_user_on_slack('Success!') + slack_client = SlackClient.new + slack_client.alert_user_to_award( + recipient_id: @user.slack_id + text: 'Success!' + ) end - - private - - def alert_user_on_slack(text) - slack_client = Slack::Web::Client.new - im_id = slack_client.im_open(user: @user.slack_id).fetch(:channel).fetch(:id) - slack_client.chat_postMessage(channel: im_id, text: text) - end end
Let’s look at the new tests:
require 'rails_helper' describe AwardsController do context '#create' do it 'awards the correct user' do stub_slack_client post :create, user_id: 3 expect(User.find(3).awards.count).to eq(1) end it 'DMs the user' do client = stub_slack_client post :create, user_id: 3 expect(client).to have_received(:alert_user_to_award).with( hash_including(text: 'Success!') ) end def stub_slack_client client = instance_double(SlackClient, alert_user_to_award: nil) allow(SlackClient).to receive(:new).and_return(client) client end end end
This test is now separated from the details of interacting with Slack’s API. If we decide to alert the user in a different way, or if the Slack API changes, this test does not need to change at all. `SlackClient`’s internals will change, and the tests for `SlackClient` will change, but the controller tests are correctly shielded from that churn.
This fixes each of the problems listed above:
There’s an additional benefit as well: refactoring will be easier since the Slack code is centralized in `SlackClient`. If we add caching or other optimizations, it will benefit everywhere that interacts with Slack. If we scattered direct `Slack::Web::Client` calls all over, it would be much harder to find all of them and refactor in a few months.
Don’t forget: if you’re using a 3rd-party API client, don’t wait: wrap it in your own class on first use!