search
LoginSignup
4
Help us understand the problem. What are the problem?

More than 1 year has passed since last update.

posted at

updated at

コメントにはコードを見てもパッとわからないことを書こう

なぜコメントを書くか

将来の開発者がより簡単に、かつより正確に、コードを利用・メンテナンスできるようにするためです。

仕事で書くほとんどのコードは将来の変更・メンテナンスが必要です。したがって仕事で書くほとんどのコードには適切なコメントが必要です。例外はメンテナンスしないコード、すなわちハッカソンやプロトタイプ目的で書いた、捨てるつもりのコードです。

コメントに何を書くか

この記事のタイトルにもしていますが、これは A Philosophy of Software Design という本に書かれているものです(13章のタイトルになっています)。

Comments should describe things that aren’t obvious from the code.

「コメントにはコードを見てもパッとわからないことを書こう」

コードを見てパッとわかることはコードを見ればいいわけです。コード外のコンテキストを知らないとわからないことや、コードを時間と労力をかけて読み込まないとわからないことを、コメントに書こうということです。

この記事の内容も概ねこの本に書かれていることを、具体例を多めに入れつつコンパクトにまとめたものです。

具体例

Interface comments / Summary doc

  • そのコンポーネント(クラス、モジュール、メソッド、関数、etc)が何をするものなのか、どういった動きをするのかを直感的に理解できる情報を書く。
    • これらの情報は、コメントやドキュメントがない限り、コンポーネントの中の実装や使われている箇所を時間をかけて読まないとわからない(パッとはわからない)。
  • 書く場所
    • コンポーネントの定義のすぐ上(外側)。
  • Tips
    • 実装よりも抽象化した情報を書く。
    • 利用時の注意点なども書いておくとよい。
    • インターフェイスとなるメソッド・関数などは、引数や返り値の説明(型や補足情報)などもあるとよい。
  • これがあると、
    • コンポーネントが何をするものなのかを知るために、コンポーネントの中の実装や、それが使われている場所を長時間かけて読まなくてよくなる。
      • コンポーネントをはじめて読むときに知りたいのは、たいていの場合、そのコンポーネントの実装がどうなっているかではなく、そのコンポーネントが何をするものなのか。それが手間をかけずにわかる状態になっているのが理想。

  • クラス
#
# ベースプラン契約が終了したときに残っているオプション契約の終了処理を行う。
#
# 指定された日付の前日に契約が終了したベースプランの契約について、同じ会社のオプション契約を status に応じて以下のように処理する。
#  - まだ開始していないものは、契約をキャンセルする。
#  - 開始して走っているものは、契約の終了日をベースプランの終了日と同じ日に変更して終了させる。同時に自動更新設定もオフにする。
#  - すでに終了しているものは、何も行わない。
#
# @see https://github.com/wantedly/company-payment/blob/master/doc/specs/base_plan_termination.md
#
# このサービスはスケジューラーで実行されて、途中で失敗したら再実行する必要があるので、このサービスの処理は冪等になっている必要がある。
#
class OptionContractsTerminationService
  def self.perform(date: Date.current)
    ended_base_contracts = CompanyPayment::Contract.plan_base.authorized.where(end_on: date.ago(1.days), auto_renew: false)
    ended_base_contracts.each do |base_contract|
      ...
  • モデル

モデルの doc には summary doc と合わせて、下(Data structure members annotation)で説明する、各カラムの説明も書いている。

# == Table Explanation
#
#  Plan プラン
#
#  商品情報を持っているマスターテーブル(会社ごとに作られない)。
#  このテーブルのレコードを作るのは社内の人間(開発者 + オペレーションメンバー)のみ。
#  同じ "プラン" (e.g. "プレミアムプラン") でも複数の期間で提供している (e.g. 3ヶ月、6ヶ月、12ヶ月など) 場合、
#  期間ごとに別のレコードを作る。
#
#  この Plan が持っている金額や自動更新するかどうかなどの情報はそのプランのデフォルト値で、
#  Contract レコードで契約単位で値をカスタマイズすることもできる。
#
#  プランの値上げ・値下げなどを行う場合は、既存のレコードの値を変更するのではなく、別のレコードを作ること。
#  既存のレコードの値を変えると、過去の売上計算の結果が正しかったかどうかわからなくなるため。
#
#  == Columns
#  * [C] と書いてあるものは Contract 作成時に初期値として Plan の値がコピーされる。
#
#  id
#  name            [C] プラン名。企業向けの請求書やメール、画面等に表示される。
#  code            プランコード。システムで申し込みなどのときにプランを特定する用途。
#                  - オペレーションメンバーが作成したものなど、システムで申し込みが行われないものには存在しない。
#  category        基本プランなら base、オプションなら option。
#  available_from  提供可能期間の開始日。
#  available_to    提供可能期間の終了日。まだ提供終了の予定がない場合は null。
#  auto_renew      [C] 契約を自動更新するかどうか。
#  term_count      [C] 契約期間の数字部分。6ヶ月なら 6。
#  term_unit       [C] 契約期間の単位部分。6ヶ月なら month。
#  currency        [C] 通貨。
#  amount          [C] 金額(総額、税抜)。月10万円の6ヶ月プランなら 600000。
#  note            社内用メモ。企業には表示されない。
#
#  created_at
#  updated_at
#  deleted_at
#

class Plan < ApplicationRecord

  • ツール系

Usage も書いてあげるとわかりやすい。

# == Summary
#
#  This custom validator validates an attribute of time-related type (datetime and date) 
#  is after another attribute in terms of time order.
#
#  This is useful when you have, for example, a pair of columns composing a term,
#  like `start_on` and `end_on`, where `end_on` must always be after `start_on`.
#
# == Usage
#
#  class Contract < ApplicationRecord
#    validates :end_on, time_order: { after: :start_on, allow_nil: true }
#  end
#
# == Options
#
#  after     [String, Symbol] Specify the name of another attribute to compare to.
#  allow_nil [Boolean]        When true, it skips validation if either the attribute
#                             or the compared attribute is nil. Default: false.
#
# == Note
#
#  Currently, it considers valid the case when the validated attribute equals
#  the compared attribute. If you want to treat it as invalid, 
#  you are welcome to add `allow_equal` option with default true.
#
#  Currently, only `after` is implemented. You are welcome to add `before`.
#
class TimeOrderValidator < ActiveModel::EachValidator
  def validate_each(record, attribute, value)
    ...
  • 関数・メソッド

引数、返り値の説明の他に、どういうものを受け取ったらどういうものを返すかの例も書いてあげると、何をするものなのか理解しやすくなることがある。

# 契約金額に固定額割引、%割引を適用して月数で按分した結果を配列で返す。これが月々の税抜売上額のベースとなる。
# 具体的には以下の計算を行う。
#
# ① 契約金額 / 按分月数 を計算し、端数は初月に寄せる。
# ② 固定額割引額 / 按分月数 を計算し、端数は初月に寄せる(初月が多く割り引き)。
# ③ (① - ②) * (1 - %割引率) を計算し、小数点以下は切り上げる。
#
# @see https://github.com/wantedly/company-payment/blob/master/doc/specs/revenue_calculation.md
#
# @param amount      [Integer] 契約金額
# @param amount_off  [Integer] 固定額割引額
# @param percent_off [Float]   %割引率。20%オフなら 0.2 を渡す。
# @param months      [Integer] 月数
#
# @return [Array<Integer>]
# @example
#   (amount: 120000, amount_off: 0,     percent_off: 0.0, months: 6) => [20000, 20000, 20000, 20000, 20000, 20000]
#   (amount: 120000, amount_off: 30000, percent_off: 0.0, months: 6) => [15000, 15000, 15000, 15000, 15000, 15000]
#   (amount: 120000, amount_off: 0,     percent_off: 0.2, months: 6) => [16000, 16000, 16000, 16000, 16000, 16000]
#
def calc_monthly_tax_excluded_revenue_amounts(amount:, amount_off: 0, percent_off: 0.0, months:)
  monthly_amounts     = divide_amount_by_months_with_remainder_to_first_month(amount, months)
  monthly_amount_offs = divide_amount_by_months_with_remainder_to_first_month(amount_off, months)
  monthly_amounts.zip(monthly_amount_offs)
    .map { |a, b| a - b }
    .map { |a| (a * (1 - percent_off)).ceil }
end

Data structure members annotation

  • テーブルのカラム、enum の値、クラス変数などが、それぞれどういう意味のものなのかを書く。
  • 書く場所
    • 宣言している場所の隣か上
  • Tips
    • 今は使われていないものがあれば、”Obsolete”, "Deprecated” などと書いておくとよい。
  • これがあると、
    • それぞれの値がどういう意味なのか、使われている箇所を調べて理解するといったことをしなくてよくなる。
    • 一つのカラム/値が想定されていなかった意味で使われるようになってしまうみたいなことを防げる。(名前にすべての文脈や意図を詰め込むことはできない。)

  • テーブルのカラム

上で説明したモデルの summary doc と一緒に書くようにしている。

# == Table Explanation
#
#  Plan プラン
#
#  商品情報を持っているマスターテーブル(会社ごとに作られない)。
#  このテーブルのレコードを作るのは社内の人間(開発者 + オペレーションメンバー)のみ。
#  同じ "プラン" (e.g. "プレミアムプラン") でも複数の期間で提供している (e.g. 3ヶ月、6ヶ月、12ヶ月など) 場合、
#  期間ごとに別のレコードを作る。
#
#  この Plan が持っている金額や自動更新するかどうかなどの情報はそのプランのデフォルト値で、
#  Contract レコードで契約単位で値をカスタマイズすることもできる。
#
#  プランの値上げ・値下げなどを行う場合は、既存のレコードの値を変更するのではなく、別のレコードを作ること。
#  既存のレコードの値を変えると、過去の売上計算の結果が正しかったかどうかわからなくなるため。
#
#  == Columns
#  * [C] と書いてあるものは Contract 作成時に初期値として Plan の値がコピーされる。
#
#  id
#  name            [C] プラン名。企業向けの請求書やメール、画面等に表示される。
#  code            プランコード。システムで申し込みなどのときにプランを特定する用途。
#                  - オペレーションメンバーが作成したものなど、システムで申し込みが行われないものには存在しない。
#  category        基本プランなら base、オプションなら option。
#  available_from  提供可能期間の開始日。
#  available_to    提供可能期間の終了日。まだ提供終了の予定がない場合は null。
#  auto_renew      [C] 契約を自動更新するかどうか。
#  term_count      [C] 契約期間の数字部分。6ヶ月なら 6。
#  term_unit       [C] 契約期間の単位部分。6ヶ月なら month。
#  currency        [C] 通貨。
#  amount          [C] 金額(総額、税抜)。月10万円の6ヶ月プランなら 600000。
#  note            社内用メモ。企業には表示されない。
#
#  created_at
#  updated_at
#  deleted_at
#

class Plan < ApplicationRecord

  • Enum
class Post < ApplicationRecord

  CATEGORIES = {
    blog:               0,  # [DEPRECATED] For "Team" (closed)
    idea:               1,  # [DEPRECATED] For "Team" (closed)
    release:            2,  # [DEPRECATED] For "Team" (closed)
    feed:               3,  # Original "Company Feed" post in simple format with cover image and plain-text body. Currently called "Update" and posted in company page.
    employee_interview: 4,  # [DEPRECATED] Intergrated to `post_article`. https://github.com/wantedly/wantedly/issues/31114
    experience_story:   5,  # Posted and shown in profile page.
    post_article:       6,  # Current "Feed" post.
    tool_review:        7,  # [DEPRECATED] Review post in "Tools"
    internal_post:      8,  # For "Internal Story"
  }
  enum category: CATEGORIES
  • クラス変数
class Contract < ApplicationRecord

  # 契約の自動更新時に前のレコードから値を引き継ぐカラム
  ATTRIBUTES_TO_INHERIT_ON_RENEWAL = [
    :company_id,
    :plan_id,
    :name,
    :auto_renew,
    :term_count,
    :term_unit,
    :currency,
    :amount,
    :invoicing_cycle,
  ]

Implementation comments

  • 実装の中でコードを見てもパッとわからないことを書く。
  • 書く場所
    • メソッドの中。実装コードの横や上
  • Tips
    • How ではなく、What や Why を書く。
      • 何をやりたいか(What)がわかれば、コードがどう動くのか(How)の理解は簡単になる。
      • 逆にどう動くのかがわかっても、なぜそのコードが必要なのかはコード外のコンテキストを知らないとわからないこともよくある。
    • 本番でエラーが起きてそれに対応するコードを入れたときなどは、その issue や PR の URL を貼っておくとよい。
  • これがあると、
    • コードをより短い時間で、正しく理解できるようになる(コードの気持ちがわかるようになる)。
    • 後の開発者が「この処理なくてもいいでしょ」と思って消してしまって事故が起きることを防げる。

  • なぜそのようなコードが書かれているのかを補う情報を書く。(Why)

パッと意図がわからないことをやっている場合

  option_contracts = Contract.plan_option.where(company_id: base_contract.company_id)
    .where('end_on > ? OR end_on IS NULL', base_contract.end_on)
    .order(start_on: :desc) # running なものから処理していくと、terminate_contract! の中で契約更新後の次期のレコードが存在したらそれも一緒に削除されるので、
                            # その後のループでそのレコードの番が来たときにすでに削除されてしまっていてエラーになる。
                            # そのため、draft や waiting なものが先に消されるように start_on の降順で処理していく。

  option_contracts.find_each do |option_contract|
    case option_contract.status
    when :running
      ContractService.terminate_contract!(option_contract, base_contract.end_on)
    when :draft, :waiting
      ContractService.cancel_contract!(option_contract)
    end
  end

どういったときに変数が null, 0 など特殊な値になるのか、も書いてあると理解の助けになることが多い。

  # 按分月数
  dividing_months = calc_dividing_months(
    start_date: contract.start_on,
    end_date:   contract.end_on,
    term_count: contract.term_count,
    term_unit:  contract.term_unit,
  )

  # 按分月数が 0 になるのは、monthly プランで、契約開始後に契約自体がキャンセルされた場合。
  # このときは、当月に計上する通常項目はなく、過去に計上済みの項目があればその消し込み項目を出力する。
  # したがって以降の売上計算は不要なのと、以降の計算で按分月数が 0 のケースを考慮する手間を省くために、このケースは先にハンドルする。
  if dividing_months == 0
    cancel_past_items_if_necessary!(contract, year, month, dividing_months)
    return
  end

  # 定価
  monthly_catalog_amounts = divide_amount_by_months_with_remainder_to_first_month(rule.amount, dividing_months)
  ...
  • この先の数行で何をやりたいか (What)
class Ticket
  def self.consume!(company:, amount:)
    # A company may have multilple Ticket records with amount remaining (e.g. purchased one and one given by campaign).
    # Consume amount from the Tickets determined by the following priority:
    #   1. Ticket with earlier expires_at (Ticket with no expires_at comes last)
    #   2. Ticket with less remaining amount
    #
    transaction do
      tickets = company.tickets.remaining.to_a
      remaining_amount = tickets.sum(&:remaining_amount)
      raise "Amount is beyond total remaining amount of the company." if amount > remaining_amount

      tickets = tickets.group_by { |t| t.expires_at.present? }
      ordered_tickets = []
      ordered_tickets += tickets[true].to_a.sort_by { |t| [t.expires_at.to_i, t.remaining_amount] }  # to_i to cut down miliseconds
      ordered_tickets += tickets[false].to_a.sort_by(&:remaining_amount)

      remainer = amount

      ordered_tickets.each do |ticket|
        break unless remainer > 0

        amount_to_consume = [remainer, ticket.remaining_amount].min
        remainer -= amount_to_consume
        ticket.consumed_amount += amount_to_consume
        ticket.save!
      end
    end
  end

コードブロックが長くなるときは、セクション化すると見通しがよくなる。

  # Step 1: Do something
  # -----------------------------------


  # Step 2: Do another 
  # -----------------------------------


  # Step 3: Do yet another
  # -----------------------------------

PRレビューのタイミングはコメントを追加するいい機会

  • request review する前に、自分で File Changes を眺めて、ここ何やってるか/なんでやってるかパッとわからないなと思ったら、コメントを追加する。
  • レビュワーに「これは何をしているんですか?」「この処理はなぜ必要ですか?」みたいに聞かれたら、コメントにも書く。
    • PR上で返答してもその2人にしか共有されないし、時間が経つと忘れてまたわからなくなる可能性がある。後の開発者(Nヶ月後の自分も含む)もわかるようにコードの方に残しておきましょう。

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
What you can do with signing up
4
Help us understand the problem. What are the problem?