RubyKaigi 2024のコードレビュー企画の概要と解説を公開します

どうも、そろそろ梅雨に入ろうかという天気が多くなって憂鬱ですね。マネーフォワードで技術広報をしている @luccafort です。 RubyKaigi 2024に参加した方も、今年は参加できなかったという方も、いかがお過ごしですか？

今年のマネーフォワードのブース設計のテーマは「Let's make it」でした。推しうちわやコードレビューなど皆さんと一緒に作り上げる！をテーマにブースを設計させていただきました。立ち寄られた方は楽しんでいただけたでしょうか？

ブース企画概要

今回マネーフォワードのブースでおこなったブース企画を簡単に説明します。

このイベントでは意図的に完璧ではない、レビューするポイントを含んだプログラムコードを展示していました。ブースに立ち寄ってくれた方に、付箋を使って「どこに問題がありそうか？」「自分ならどうするか？」を考え、レビューしてもらうことを目的とした企画です。このコードはDay1からDay3にかけて毎日新しいものに更新され、前日回答した方も、前日に解答できなかった方も、毎日チャレンジできるように配慮されていました。

本記事ではRubyKaigi 2024のブースで掲載したコードとレビューコメントの一部を公開させていただきました。よければ皆さんもレビューや、自分が書いたコメントが意図されたものだったかをお楽しみください。

code_reviewers

Day1

code review day1

出題者: 山下

Day 1に出題したコードのテーマは次のとおりです。

従業員モデルの情報をCSVとして出力するコードです。

Code Review

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

  gem "activerecord"
  gem "sqlite3"
  gem "minitest"
end

require "active_record"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "sample")

ActiveRecord::Schema.define do
  create_table :office_members do |t|
    t.string :name
    t.integer :number
    t.integer :role, default: 0

    t.timestamps
  end
end

class UserClient
  class << self
    # Get email from another microservice by using API.
    # eg. 'example@example.com'
    def get_email(id)
      # nothing
    end
  end
end

class OfficeMember < ActiveRecord::Base
  enum role: { normal: 0, admin: 1 }
  def email
    UserClient.get_email(id)
  end

  def role_name
    if role == 'admin'
      'Administrator'
    else
      'Normal user'
    end
  end
end

require 'csv'
class ExportOfficeMembersService
  class << self
    def execute
      csv_data = CSV.generate do |csv|
        csv << ['Number', 'Name', 'Email', 'Role']
        OfficeMember.all.each do |member|
          csv << [member.number, member.name, member.email, member.role_name]
        end
      end
      File.write('office_members.csv', csv_data)
    end
  end
end

レビューコメントへのお返事

こんにちは、1日目の問題制作者の山下です。
初日のエクスポート問題のコードレビューにご参加いただきありがとうございます。

この問題については、ちょっとした裏話があります。
コードレビュー企画時、議論の中で「RubyKaigiの参加者であるならば、Ruby on Railsも触っている人が多いだろう」という意見が出ていました。そこから着想を得て、「Webサービスではエクスポート機能があるはず。このエクスポート機能について、改善・修正をした人やコードリーディングした人も少なからずいるだろう。その知見をコードレビューとしてまとめるとおもしろそう」と考えました。
またコード作成に関しては、GitHub Copilotを使った自動生成のコードも織り交ぜました。そのうちの1つはExportOfficeMembersServiceクラスです。動きそうな形になりつつも、気になる点があるコードができたと思います。

おかげで、多くのコメントが寄せられたコードレビューとなりました。このなかからいくつか気になったコメントをピックアップします。

【1】ループ内でRemote Procedure Callしている

csv << [member.number, member.name, member.email, member.role_name]のmember.emailの箇所ですね。実装内容を見てみると、他のマイクロサービスからemailを取得する処理となっています。この呼び出しをループ内に入れた場合、APIコールが多くなってしまいます。ループ前に取得するといった変更をするのが良さそうですね。

【2】`role_name`メソッドでの権限名取得方法

ifの判定条件の改善やi18nへの対応など、多くのコメントが寄せられました。roleにenumを使っていたこともあり、「もっとシンプルに書けるのでは」というコメントもありました。付箋の多くがここに集中しているようでしたので、参加者の中にはこの書き方に関する指摘や修正経験を持っていたのかもしれません。

【3】メールアドレス取得APIが失敗した時のエラーハンドリング

これは予想していませんでした。指摘されて確かにそうだと思ったポイントです。
このコードではそのメールアドレス取得失敗の他に、エクスポートファイルの読み書き失敗などがあり得るので、そこに対するエラーハンドリングが必要ですね。

Day2

code review day2

出題者: k0i

Day 2に出題したコードのテーマは次のとおりです。

次のコードは、一日の終わりにアカウントの残高と支払いの合計額を計算するモデルを表しています。

Code Review

# frozen_string_literal: true

WORKER_THREAD_NUM = 5

# calculate the total amount of payments and the remaining balance for each account at the end of the day
class DailySettlement
  def execute
    target_accounts.each do |account|
      paid_sum = calculate(account:)
      account.save!
      notify! account, paid_sum
    end
  end

  def calculate(account:)
    paid_sum = 0
    target_payments = Payment.unsettled(account_id: account.id, created_at: Date.yesterday.all_day)
    WORKER_THREAD_NUM.times.map do
      Thread.new do
        while (payment = target_payments.pop)
          if account.balance < payment.amount
            # we check the balance whenever a payment is created, so this block should never be reached
            critical! "Account #{account.id} has insufficient balance; Needs to be settled manually"
            break
          end
          paid_sum += payment.amount
          account.balance -= payment.amount
        end
      end
    end.map(&:join)

    target_payments.update_all(status: :settled)

    paid_sum
  end

  def notify!(account, paid_sum)
    SlackNotifier.notify("Account #{account.id} has been settled. Balance: #{account.balance}, Paid: #{paid_sum}")
  end

  def target_accounts = Account.paid_today
end

レビューコメントへのお返事

こんにちは、2日目の問題制作者のk0iです。

問題を作るに当たって、「折角のRubyKaigiなのでrubyの特徴を盛り込んだもの」と「誰でもレビューに参加できる、ぱっと見でfishyなもの」をどう両立させるかを結構悩み、結果ツッコミどころのあるコードを書きつつ更にGVL（Global VM Lock）周りにも触れて策問することにしました。

沢山のレビューを頂き、ありがとうございました。

いくつか頂いたレビューを紹介します。

「変数の更新があるので排他制御したい」、「arrayってthread safeではないのでは」と排他制御に関するレビューを多くいただきました。

出題コードは排他制御を行なっておらず、Threadでの処理中にputs等のGVLを手放すIO処理を入れると毎回異なる結果になってしまいます。ご指摘いただいた通り、GVLが解放されても大丈夫なようにaccount.balanceへのアクセスをMutexで排他制御をしたり、Arrayの代わりにQueueクラスを利用する必要があります。

また、処理するデータ量が増えた時を懸念するレビューが多かったのも印象的でした。

例えば「eachではなくfind_eachを使った方が良い」というレビューを頂きましたが、これもご指摘いただいた通りで、eaсhだとメモリに乗り切らない恐れがありますので、find_eachでbatch_sizeを指定するべきところです。

他には、「Slackに通知を送ってるけど、Slackはカジュアルにrate limitに到達する」というものを頂きました。 Slackのレートリミットは正直に申し上げますと全然想定しておらず、「なるほど確かに....！」と思いました。良い勉強の機会になりました。ありがとうございました。

「ブース横で立ったままレビューする」という、なかなか集中するのは難しい環境でしたが、他にも多くの「その観点はなかったな〜」というレビューを頂き、Rubyistの皆さんのレビュー力に感嘆するばかりでした。

重ねてレビューしてくださった方々、ありがとうございました。

Day3

code review day3

出題者: えりりん

Day 3に出題したコードのテーマは次のとおりです。

これはユーザーがファイルをアップロードできるアプリケーションのコードの一部です。
FilesController#index はユーザーの role に合わせて index ページに表示する file のデータを取得するメソッドです。
このコードをレビューしてください。

Code Review

ActiveRecord::Schema.define do
  create_table :user, force: true do |t|
    t.bigint :id, null: false
    t.string :name, null: false
    t.integer :company_id, null: false
    t.integer :role, null: false
  end
  create_table :file, force: true do |t|
    t.bigint :id, null: false
    t.string :file_name, null: false
    t.integer :user_id, null: false
    t.string :file_image_url, null: false
  end
  create_table :approver_applicant, force: true do |t|
    t.bigint :id, null: false
    t.integer :approver_user_id, null: false
    t.integer :applicant_user_id, null: false
  end
end

module SessionHelper
  def current_user
    @current_user = @current_user || User.find_by(id: session[:user_id])
  end
end

class User < ActiveRecord::Base
  enum(
    role: {
      admin: 0,
      approver: 1,
      viewer: 2
    }
  )
end
class ApproverApplicant < ActiveRecord::Base
end
class File < ActiveRecord::Base
end

class ApplicationController < ActionController::Base
  include SessionHelper
end
class FilesController < ApplicationController
  def index
    same_company_user_ids = User.where(company_id: current_user.company_id).pluck(:id)
    @files = []
    # 同じ会社の全員のファイルを取得できる
    # get same company's members' files
    if current_user.role == 0
      @files = File.where(user_id: same_company_user_ids)
    end
    # 自分が承認者となる人のファイルを取得できる
    # when the user is approver, get the user's applicants create files
    if current_user.role == 1
      applicant_user_ids = ApproverApplicant.where(approver_user_id: current_user.id).pluck(:applicant_user_id)
      @files = File.where(user_id: applicant_user_ids)
    end
    # 自分のファイルだけ取得できる
    # get own files
    if current_user.role == 2
      @files = @files.where(user_id: current_user.id)
    end
    @files
  end
end

レビューコメントへのお返事

こんにちは、3日目の問題制作者のえりりんです。コードレビューにご参加いただきありがとうございました！ migration ファイルの部分、モデルの部分、Controller のロジック、私が最後まで気づけなかった typo までご指摘いただいて、細かく見ていただけたんだな〜と嬉しかったです。いくつかいただいたレビューにお答えしていきたいと思います。

【1】Userのロール判定の if 文

ここは違和感を覚えた方も多かったのではないでしょうか。「case 文にする、if ~ elsif を使う。また、admin かどうかの判定については、admin? というメソッドを別途用意して判定するのが良さそう」という意見もありました。いずれもそちらのほうがよく見るので修正したいですね。

【2】同じ会社のUserを取得する部分

ユーザーが多いと処理できなくなりそうとのコメントがありました。確かにそうですね。どうするとより良くなるのかはもっと議論したいなと思いました。また、この部分を別メソッドに切り出すのが良さそうというコメントもありました。

【3】migration ファイルへのレビュー

users テーブルの role カラムにデフォルト値を持つの良さそうだなと思いました。少なくとも一般ユーザーではあるはずなので、そこで担保するのもありですね。 files テーブルで user_id と file_image_url にユニークインデックスを貼るのはどうかというコメントがありました。これは、file_image_url がどういうものが発行されるかにもよりますし、実装前により深い仕様を確認する必要がありそうです。もっと考えられることがあるなと気づかせていただきました。

アプリケーションの一部を表しただけのコードでもこれだけコメントがいただけて嬉しかったです。ではどうするのが良いだろう、とさらに議論したいポイントもありました。

コメントしていただいた方、もし読んでいただけましたらどこかで議論しましょう！笑
疲れがたまっていたであろう3日目でもブースにお立ち寄りいただき、ありがとうございました！

まとめ

昨年に引き続き3日間を通して、多くのコメントをいただきました🎉 「初日に書こうと思っていたコメントがすでに書かれているのが悔しかったので2日目は早めにきました！」というコメントをいただくこともありました。ブースに立ち寄ってくれた多くのRubyistの皆さんに楽しんでいただけたのではないでしょうか？

普段の開発で忙しい中、コードレビュー用のサンプルコードも実装し、レビューまでしていただき、3人には本当に感謝しています。ありがとうございました！ RubyKaigi 2024に来れなかった方もぜひこちらのコードをレビューして、実装者も思いつかないような意外なミスや指摘をしてもらえると嬉しいです。

thank you RubyKaigi 2024

Money Forward Developers Blog

株式会社マネーフォワード公式開発者向けブログです。技術や開発手法、イベント登壇などを発信します。サービスに関するご質問は、各サービス窓口までご連絡ください。

RubyKaigi 2024のコードレビュー企画の概要と解説を公開します

ブース企画概要

Day1

Code Review

レビューコメントへのお返事

【1】ループ内でRemote Procedure Callしている

【2】`role_name`メソッドでの権限名取得方法

【3】メールアドレス取得APIが失敗した時のエラーハンドリング

Day2

Code Review

レビューコメントへのお返事

Day3

Code Review

レビューコメントへのお返事

【1】Userのロール判定の if 文

【2】同じ会社のUserを取得する部分

【3】migration ファイルへのレビュー

まとめ

ブース企画概要

Day1

Code Review

レビューコメントへのお返事

【1】ループ内でRemote Procedure Callしている

【2】role_nameメソッドでの権限名取得方法

【3】メールアドレス取得APIが失敗した時のエラーハンドリング

Day2

Code Review

レビューコメントへのお返事

Day3

Code Review

レビューコメントへのお返事

【1】Userのロール判定の if 文

【2】同じ会社のUserを取得する部分

【3】migration ファイルへのレビュー

まとめ

【2】`role_name`メソッドでの権限名取得方法