In Progress
Unit 1, Lesson 1
In Progress

No Comment

Video transcript & code

One of the ongoing debates in software development is the question of how often to use comments. On the one hand, judicious use of comments can clarify the intent of code. On the other hand, a comment that is out-of-date and inaccurate can be worse than no comment at all.

I was updating an app of mine recently, and I ran across a line of code that brought this age-old debate to mind.

I had left myself a lengthy comment, to explain a particular idiosyncrasy of the third-party API I was using. I'll give you a moment to read it.

# ...
user = DB[:users][email_address: email]
github_id = Integer(user.fetch(:github_id))
gh_client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_APP_TOKEN"))
gh_user   = gh_client.user(github_id)
gh_login  = gh_user.login
team_id   = Integer(ENV.fetch("GITHUB_TEAM_ID"))
logger.info "Adding user #{gh_login} (##{github_id}) to team #{team_id}"
# Fun fact: the team_id MUST be an integer ID,
# while the user MUST be specified as a string login.
# Consistency is the hobgoblin of small minds.
response = gh_client.add_team_membership(team_id, gh_login)
# ...

Now I'll confess that I sometimes leave these little notes just to amuse myself. But it got me thinking again about the argument that all code should be sufficiently self-explanatory that no comment is required. What would it take to make this comment superfluous? Is that even possible?

One of the biggest problems with this comment as it stands now is that it's in the middle of some business logic. That means that it's quite possible I might use the same API call somewhere else, but forget the important explanatory comment.

So our first step might be to extract out this code into a reusable method. We'll create a new class, which functions as an adapter to isolate and contain knowledge about the third-part API being used here.

class GithubGateway
end

A quick note on terminology: I've said this is an "adapter", which is the term Alistair Cockburn uses in his "hexagonal architecture". I've also named the class a "gateway", which is Martin Fowler's term. Sandi Metz has a third name for this kind of wrapper class: an "outerface", as contrasted with the "interface" that our code exposes to clients.

We proceed to extract the portion of the code that deals with Github into a new method called #add_user_to_team.

class GithubGateway
  def add_user_to_team
    gh_client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_APP_TOKEN"))
    gh_user   = gh_client.user(github_id)
    gh_login  = gh_user.login
    team_id   = Integer(ENV.fetch("GITHUB_TEAM_ID"))
    logger.info "Adding user #{gh_login} (##{github_id}) to team #{team_id}"
    # Fun fact: the team_id MUST be an integer ID,
    # while the user MUST be specified as a string login.
    # Consistency is the hobgoblin of small minds.
    response = gh_client.add_team_membership(team_id, gh_login)
  end
end

This gives us an opportunity to add some highly self-explanatory keyword arguments.

We rename github_id to github_user_id, and add it as a required keyword argument.

We rename team_id to github_team_id, and add it as another required argument.

We also provide a way to inject the logger, along with a reasonable default.

class GithubGateway
  def add_user_to_team(github_user_id:,
                       github_team_id:,
                       logger: Logger.new($stderr))
    gh_client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_APP_TOKEN"))
    gh_user   = gh_client.user(github_user_id)
    gh_login  = gh_user.login
    logger.info "Adding user #{gh_login} (##{github_user_id}) to team #{team_user_id}"
    # Fun fact: the team_id MUST be an integer ID,
    # while the user MUST be specified as a string login.
    # Consistency is the hobgoblin of small minds.
    response = gh_client.add_team_membership(github_team_id, gh_login)
  end
end

We can now retrofit the original code to use this adapter.

# ...
user = DB[:users][email_address: email]
github_id = Integer(user.fetch(:github_id))
team_id   = Integer(ENV.fetch("GITHUB_TEAM_ID"))
GithubGateway.new.add_user_to_team(
  github_user_id: github_id,
  github_team_id: team_id,
  logger: logger)

Back to our adapter method, we decide to take this extraction as a chance to make things even more self-documenting.

We add some conservative coercion to the user and team ID parameters, using the Integer() conversion function.

class GithubGateway
  def add_user_to_team(github_user_id:,
                       github_team_id:,
                       logger: Logger.new($stderr))
    github_user_id = Integer(github_user_id)
    github_team_id = Integer(github_team_id)
    gh_client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_APP_TOKEN"))
    gh_user   = gh_client.user(github_user_id)
    gh_login  = gh_user.login
    logger.info "Adding user #{gh_login} (##{github_user_id}) to team #{team_user_id}"
    # Fun fact: the team_id MUST be an integer ID,
    # while the user MUST be specified as a string login.
    # Consistency is the hobgoblin of small minds.
    response = gh_client.add_team_membership(github_team_id, gh_login)
  end
end

As you might recall from episode #207, this conversion function will pass integers through, as well as converting string representations of integers.

Integer(23)                     # => 23
Integer("42")                   # => 42

But unlike the lenient #to_i method, it will not permit arbitrary strings.

"Team Blue".to_i                # => 0
Integer("Blue Team")            # =>


# ~> ArgumentError
# ~> invalid value for Integer(): "Blue Team"
# ~>
# ~> xmptmp-in4866Mff.rb:2:in `Integer'
# ~> xmptmp-in4866Mff.rb:2:in `<main>'

Similarly, it won't allow nils through.

nil.to_i                        # => 0
Integer(nil)                    # =>


# ~> TypeError
# ~> can't convert nil into Integer
# ~>
# ~> xmptmp-in4866yRH.rb:2:in `Integer'
# ~> xmptmp-in4866yRH.rb:2:in `<main>'

The conversion function raises a TypeError when it can't convert, which is a pretty clear indication that there has been a violation of a stated interface.

We don't stop there. We're pretty sure that zero is not a valid team or user ID. And it's worth checking for, since as we saw a moment ago, there's always a possibility that a inadvertent #to_i call further up the line converted a bad ID string or a nil value into zero.

So we add some checks that the IDs are nonzero, along with explanatory failures if they are.

class GithubGateway
  def add_user_to_team(github_user_id:,
                       github_team_id:,
                       logger: Logger.new($stderr))
    github_user_id = Integer(github_user_id).nonzero? or
      fail ArgumentError, "User ID must be a non-zero integer"
    github_team_id = Integer(github_team_id).nonzero? or
      fail ArgumentError, "Team ID must be a non-zero integer"
    gh_client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_APP_TOKEN"))
    gh_user   = gh_client.user(github_user_id)
    gh_login  = gh_user.login
    logger.info "Adding user #{gh_login} (##{github_user_id}) to team #{team_user_id}"
    # Fun fact: the team_id MUST be an integer ID,
    # while the user MUST be specified as a string login.
    # Consistency is the hobgoblin of small minds.
    response = gh_client.add_team_membership(github_team_id, gh_login)
  end
end

As you might remember from episode #294, the nonzero? predicate has the unusual behavior that when it is true, it returns the number itself instead of the value true.

23.nonzero?                     # => 23

So this code will permit inputs that are unambiguously convertible to non-zero integers, and reject all others.

We have now armored this method with both static and dynamic documentation about its inputs. Statically speaking, the very explicit parameter names, conversions, and guard clauses at the top of the method say as much as any comment could about the expected types. And dynamically, if the expected input types are not received, a reasonably explanatory exception will immediately be raised.

But we've gotten a bit distracted. Originally, we were looking for a way to eliminate this explaining comment about an inconsistency in the way the Octokit API expects arguments.

# Fun fact: the team_id MUST be an integer ID,
# while the user MUST be specified as a string login.
# Consistency is the hobgoblin of small minds.
# ...

And while we've pulled this comment out of the main flow of business logic, we haven't really removed its reason for existence.

It's confession time: quite honestly, as comments go I think this one is more justified than most. I think so long as a comment like this is isolated to one place in the code, and that place is a method whose entire job is to act as an impedance-matching layer between our application and a third-party library, there's a strong argument for keeping it around. There's a lot of ceremony around calling this API method, and the comment draws attention to why.

But if we really wanted to get rid of the comment without losing information, how would we do so? Here's one suggestion. We give the variable containing the Github user login string a new name. A name that explicitly states the reason we need it.

At this point, we can get rid of the comment, safe in the knowledge that our explanatory variable name will act as a signpost to later readers.

gh_login_as_required_by_octokit  = gh_user.login
logger.info "Adding user #{gh_login_as_required_by_octokit} (##{github_user_id}) to team #{team_user_id}"
response = gh_client.add_team_membership(github_team_id,
                                         gh_login_as_required_by_octokit)

I'm not going to tell you that you should get rid of all comments everywhere. That said, any time you run across a comment and think, "oh, THIS comment needs to be there", it's a good moment to pause and reflect. Does it really, truly need to be there? Is there some way you could rewrite the code to explain itself more clearly?

I find the answers to these questions often yield new insights into how to construct my methods. Perhaps they will do the same for you as well.

Happy hacking!

Responses