How much or how little do you comment or document your code?

opinions

#1

Inspired by @joeerl’s talk posted here, how much or how little do you comment or document your code?

If you’d like to post examples, that would be cool too!

If you haven't seen the talk I recommend it, but in short here are some spoilers from it (click the arrow or this sentence to reveal!):
  • Joe thinks we should comment - and comment well!
  • Joe said that @rvirding is famously known for his single comment while making Erlang. While writing the code for the compiler for pattern matching, in the middle of it there was a single line that said:
% Now for the tricky bit

:043:


#2

I comment way to little, I often don’t comment modules of document my functions. I look back at projects I wrote earlier in the year and often have a hard time understanding them which can be frustrating.

I do however often create flowcharts and pseudo-code alongside the project so that there is something to run through, which is quite useful for both myself and others who look at the code.

I’m trying to start commenting more things though, and document all my functions, which is slowly taking effect over my projects.


#3

I think comment usefulness depends on how stable the code is, really. If it’s a really active code base with a lot of new people joining in regularly I find comments that have the potential to get out of sync to be error prone and hurt more than they help. Now if your comment is a unit test and you have good testing practices in place, maybe the comments can be more helpful than harmful?

My default is to rethink variable and function names before resorting to a comment, though. Something detached from any pipeline/runtime errors can go stale really quickly and confuse future developers.


#4

When I started Ruby, I used to comment - but kept them brief - because I thought that was what we were meant to do :lol:

Then I came across a Ruby Gem by Nando Vieira and was really impressed by how well documented it was:

module PayPal
  module Recurring
    class Base
      attr_accessor :amount
      attr_accessor :cancel_url
      attr_accessor :currency
      attr_accessor :description
      attr_accessor :note
      attr_accessor :email
      attr_accessor :failed
      attr_accessor :frequency
      attr_accessor :initial_amount
      attr_accessor :initial_amount_action
      attr_accessor :ipn_url
      attr_accessor :locale
      attr_accessor :outstanding
      attr_accessor :payer_id
      attr_accessor :period
      attr_accessor :profile_id
      attr_accessor :reference
      attr_accessor :refund_type
      attr_accessor :return_url
      attr_accessor :start_at
      attr_accessor :token
      attr_accessor :transaction_id
      attr_accessor :item_category
      attr_accessor :item_name
      attr_accessor :item_amount
      attr_accessor :item_quantity
      attr_accessor :trial_frequency
      attr_accessor :trial_length
      attr_accessor :trial_period
      attr_accessor :trial_amount

      def initialize(options = {})
        options.each {|name, value| send("#{name}=", value)}
      end

      # Just a shortcut convenience.
      #
      def request # :nodoc:
        @request ||= Request.new
      end

      # Request a checkout token.
      #
      #   ppr = PayPal::Recurring.new({
      #     :return_url         => "http://example.com/checkout/thank_you",
      #     :cancel_url         => "http://example.com/checkout/canceled",
      #     :ipn_url            => "http://example.com/paypal/ipn",
      #     :description        => "Awesome - Monthly Subscription",
      #     :amount             => "9.00",
      #     :currency           => "USD"
      #   })
      #
      #   response = ppr.request_token
      #   response.checkout_url
      #
      def checkout
        params = collect(
          :locale,
          :amount,
          :return_url,
          :cancel_url,
          :currency,
          :description,
          :ipn_url,
          :item_category,
          :item_name,
          :item_amount,
          :item_quantity
        ).merge(
          :payment_action => "Authorization",
          :no_shipping => 1,
          :L_BILLINGTYPE0 => "RecurringPayments"
        )

        request.run(:checkout, params)
      end

      # Suspend a recurring profile.
      # Suspended profiles can be reactivated.
      #
      #   ppr = PayPal::Recurring.new(:profile_id => "I-HYRKXBMNLFSK")
      #   response = ppr.suspend
      #
      def suspend
        request.run(:manage_profile, :action => :suspend, :profile_id => profile_id)
      end

      # Reactivate a suspended recurring profile.
      #
      #   ppr = PayPal::Recurring.new(:profile_id => "I-HYRKXBMNLFSK")
      #   response = ppr.reactivate
      #
      def reactivate
        request.run(:manage_profile, :action => :reactivate, :profile_id => profile_id)
      end

      # Cancel a recurring profile.
      # Cancelled profiles cannot be reactivated.
      #
      #   ppr = PayPal::Recurring.new(:profile_id => "I-HYRKXBMNLFSK")
      #   response = ppr.cancel
      #
      def cancel
        request.run(:manage_profile, :action => :cancel, :profile_id => profile_id)
      end

      # Return checkout details.
      #
      #   ppr = PayPal::Recurring.new(:token => "EC-6LX60229XS426623E")
      #   response = ppr.checkout_details
      #
      def checkout_details
        request.run(:details, :token => token)
      end

      # Request payment.
      #
      #   # ppr = PayPal::Recurring.new({
      #     :token       => "EC-6LX60229XS426623E",
      #     :payer_id    => "WTTS5KC2T46YU",
      #     :amount      => "9.00",
      #     :description => "Awesome - Monthly Subscription"
      #   })
      #   response = ppr.request_payment
      #   response.completed? && response.approved?
      #
      def request_payment
        params = collect(
          :amount,
          :return_url,
          :cancel_url,
          :ipn_url,
          :currency,
          :description,
          :payer_id,
          :token,
          :reference,
          :item_category,
          :item_name,
          :item_amount,
          :item_quantity
        ).merge(:payment_action => "Sale")

        request.run(:payment, params)
      end

      # Create a recurring billing profile.
      #
      #   ppr = PayPal::Recurring.new({
      #     :amount                => "9.00",
      #     :initial_amount        => "9.00",
      #     :initial_amount_action => :cancel,
      #     :currency              => "USD",
      #     :description           => "Awesome - Monthly Subscription",
      #     :ipn_url               => "http://example.com/paypal/ipn",
      #     :frequency             => 1,
      #     :token                 => "EC-05C46042TU8306821",
      #     :period                => :monthly,
      #     :reference             => "1234",
      #     :payer_id              => "WTTS5KC2T46YU",
      #     :start_at              => Time.now,
      #     :failed                => 1,
      #     :outstanding           => :next_billing,
      #     :trial_period          => :monthly,
      #     :trial_length          => 1,
      #     :trial_frequency       => 1,
      #     :trial_amount          => 0.00
      #   })
      #
      #   response = ppr.create_recurring_profile
      #
      def create_recurring_profile
        params = collect(
          :amount,
          :initial_amount,
          :initial_amount_action,
          :currency,
          :description,
          :payer_id,
          :token,
          :reference,
          :start_at,
          :failed,
          :outstanding,
          :ipn_url,
          :frequency,
          :period,
          :email,
          :trial_length,
          :trial_period,
          :trial_frequency,
          :trial_amount,
          :item_category,
          :item_name,
          :item_amount,
          :item_quantity
        )
        request.run(:create_profile, params)
      end

      # Update a recurring billing profile.
      #
      #   ppr = PayPal::Recurring.new({
      #     :amount                => "99.00",
      #     :currency              => "USD",
      #     :description           => "Awesome - Monthly Subscription",
      #     :note                  => "Changed plan to Gold",
      #     :ipn_url               => "http://example.com/paypal/ipn",
      #     :reference             => "1234",
      #     :profile_id            => "I-VCEL6TRG35CU",
      #     :start_at              => Time.now,
      #     :outstanding           => :next_billing
      #   })
      #
      #   response = ppr.update_recurring_profile
      #
      def update_recurring_profile
        params = collect(
          :amount,
          :currency,
          :description,
          :note,
          :profile_id,
          :reference,
          :start_at,
          :outstanding,
          :ipn_url,
          :email
        )

        request.run(:update_profile, params)
      end

      # Retrieve information about existing recurring profile.
      #
      #   ppr = PayPal::Recurring.new(:profile_id => "I-VCEL6TRG35CU")
      #   response = ppr.profile
      #
      def profile
        request.run(:profile, :profile_id => profile_id)
      end

      # Request a refund.
      #   ppr = PayPal::Recurring.new({
      #     :profile_id => "I-VCEL6TRG35CU",
      #     :transaction_id => "ABCEDFGH",
      #     :reference      => "1234",
      #     :refund_type    => :partial,
      #     :amount         => "9.00",
      #     :currency       => "USD"
      #   })
      #   response = ppr.refund
      #
      def refund
        params = collect(
          :transaction_id,
          :reference,
          :refund_type,
          :amount,
          :currency,
          :note
        )

        request.run(:refund, params)
      end

      private
      # Collect specified attributes and build a hash out of it.
      #
      def collect(*args) # :nodoc:
        args.inject({}) do |buffer, attr_name|
          value = send(attr_name)
          buffer[attr_name] = value if value
          buffer
        end
      end
    end
  end
end

And started to do the same. Then some time afterwards DHH tweeted/blogged how he rarely comments, but instead gives his methods (i.e functions) long descriptive names - even if they get quite long.

If you work hard at being clear in your naming, you’ll also rarely need to write comments for the code. Comments are generally only needed when you failed to be clear enough in naming. Treat them as a code smell.

Here’s some of his examples:

def make_person_an_outside_subscriber_if_all_accesses_revoked
  person.update_attribute(:outside_subscriber, true) if person.reload.accesses.blank?
end

def shift_records_upward_starting_at(position)
  positioned_records.update_all "position = position - 1",
    ["position >= ?", position]
end

def someone_else_just_finished_writing?(document)
  if event = document.current_version_event
    !event.by_current_creator? and event.updated_at > 1.minute.ago
  end
end

So I thought I’d try that. I regretted it :lol: I think having good function names is great, but good comments are incredibly useful too.


#5

In my opinion this only applies to comments which are superfluous anyway because the describe what is being done not why it is being done. They don’t give context or explanations. Obviously you should try to have good function names regardless.

I don’t mind having half a page of documentation for a function if required. It may contain links to the specification being implemented, specific trade offs taken into account. Explaining other type of information that cannot be inferred by just looking at the code and that would help me and other developers reading the code.

These are the things you cannot fit into a function name and where comments are a must. They are also what help me the most in terms of getting up to speed with a code base.


#6

I comment a lot more when I’m newer to the language and framework, just so I don’t forget what’s going on, but generally I try to keep comments to a minimum but spend a good amount of time on documentation (especially if it’s open source).


#7

During development I’ve found in most cases writing the @moduledoc with a basic 1-2 sentence summary and description of responsibilities in advance of implementation helpful in most cases.

I find this doc-driven approach useful to force thinking about contexts and responsibility before investing too much time. There’s sometimes better naming, abstractions, and more specific modules to break out lurking in the attempt to describe.

I also tend to find TODO's in the moduledoc. Not sure how they get there though. :wink:


#8

That kind of stuff should be @doc and doctests in Elixir, not comments at all.

Overall I think that any ‘good’ comment should be documentation, not a comment. The only times I really use comments are to leave markers (like TODO or FIXME or so for easy greppability), or to temporarily (I hope) comment out code for comparisons and testing.


#9

You should be using schism instead :wink:


#10

Lol, yes I know, but that doesn’t help me in other languages though. :wink:


#11

I bet you can abuse the C++ preprocessor to do what schism does and even more xD And in OCaml you can probably hack something too


#12

Already do excessively, there’s a PPX for it for OCaml… ^.^;


#13

I wrote down my thoughts on comments extensively here: Are comments a code smell? Yes! No? It Depends.

In short:

  • Documentation != Comments, I like to have module level comments for most non trivial modules to provide business context (in our case including how the German business owners will call it in German) and some application context. Of course for OSS libraries this is entirely different.
  • WHAT comments are usually bad, WHY comments are a lot better (for instance weird business rules, weird behaviour of third parties… tricky bugs etc.)
  • I treat code comments as defeat. Whenever I want to write a comment I work really hard to make the code more readable - extracting one line methods to be intention revealing and making sure that a function works on the same level of abstraction. After all these refactorings most comments are unnecessary.
  • As always context matters, new language/framework/business domain especially when working in a team potentially warrants more comments

One of my favorite quotes regarding code comments is Kent Back in “Smalltalk Best Practice Patterns” where he says he likes to comment between 0 and 1% of his code (“Oh the uproar!”) and then as a “sanity check” asks a dev of a company he’s consulting for how many methods are commented in their system are commented and he responds “0-1%” and says that it never has been a problem.

Regarding this thread, @cmkarlsson mentioned that a function might have a lot of documentation including links to the specification it implements. I disagree (slightly). Format specific functions should be contained within a single module and those links etc. should be in the module documentation imo, not the function.


#14

I remember looking at the Symbolics lisp machine code and discovered they also loved long function names but they could also include types as well. The longest I came across was 52 characters. But they also has comments as well.


#15

I think a comments and documentation are not necessarily the same thing. I see documentation as being for the USER of the module/function and comments for the IMPLEMENTER/MAINTAINER of the module/function. The documentation is what would fit in a separate documentation file or webpage.

Both have there place but are not the same and should be kept separate.


#16

True true, though for, say, elixir, I wish we had an attribute on @doc tags to imply internal documention so we could have both internal and external, would be convenient.


#17

I think having too much documentation in the source file can make it difficult to read the code. I would prefer to have the comments (for implementer and maintainer) together with the code while having the documentation (for user) in a separate place. I will admit that this is what I am used to.


#18

You might like to try my doktor package (not on hex yet), which targets your preferences. You can write your docs as a markdown file with special comments and doktor will extract them from the file at compile time and add add them as @doc attributes.


#19

I’m not too hot about those Gem code examples. :slight_smile: They’re communicating three things:

  1. How to use the functions. But tests are the right way to do this: they’re “living” comments, showing how the code is to be used. Note that the comment for checkout() seems to be outdated, documenting how to use checkout_url() instead. (?) That wouldn’t happen in a test suite.
  2. The real thing the function is doing. I.e., “Suspend a recurring profile” for suspend(). There, I would use DHH’s Clean Coding strategy and use the comment as the function name: suspend_recurring_profile().
  3. They document the union types of the class’s internal state, to use the functions. I.e., they’re making up for lack of a good type system. (Lame, but unavoidable in Ruby, I think.)

Now what the comments should really be talking about: invariants that won’t become out of sync with implementation changes. E.g., intentions, responsibilities, and policies.


#20

Given a choice I would use Literate Programming. Design, architecture, documentations, comments and code all rolled into one.

However, I would take a good @spec over comments any day.