Tech Racho エンジニアの「？」を「！」に。
  1. TOP
  2. 開発
  3. Rails: ループ内の手続処理を止めてみよう
  • 開発

Rails: ループ内の手続処理を止めてみよう

いよいよ年末になりました。1年が短く感じる今日この頃です。
今年はいろいろあったなー(結婚したり子供が産まれたり😊)

お久しぶりの記事ですが、今回は新人君のコードをレビューしたら、こんな感じでループ処理書いてもいいのでは？とおもったのでまとめてみました。

長文ですがお付き合いください🙇

新人君のコードとレビュー

まず新人君は、とあるシステムでユーザー登録を実装していました。要件は以下の通りです

1. ユーザー登録の要件

  • ユーザーは、システムにメールアドレスを入力する
  • システムは、ユーザー登録時にパスワードを自動発行する
  • システムは、メールアドレスとパスワードをDBに永続化する
  • システムは、入力されたメールアドレスに、発行したパスワードを即時送信する
  • 1度DBに永続化したパスワードは、再び参照することができない

2. 新人君のコード

以下が新人君のコード(をだいぶ添削したもの)です。ご覧ください。

class User < ApplicationRecord
  def registration!
    password = PasswordGenerator.gen(email)
    self.password = password
    save!
    RegistrationMailer.send_mail(email: email, password: password)
  end
end

########## メイン処理 ##########

User.new(email: 'hoge@example.com').registration!

Railsのコードを前提としていますが、以下のコードをrequireすると素のRubyでも動作が確認できます。以降の実行例もrequireしてある前提です。

3. 追加要求発動: それバッチでもやりたい

1ユーザーの登録ができたのですが、ディレクターからは当然のごとく
初期登録用に複数のユーザーを一気に登録したい要件が追加されました

4. ユーザー一括登録の要件

  • 管理者は、システムにメールアドレスリストを入力する
  • ユーザー登録要件に従い、各メールアドレスに対してユーザー登録を行う
  • 一括登録中にエラーが発生した場合は全ての処理を行われなかったことに(ロールバック)する

5. 新人君のコード（実装後）

ということで、新人君は以下のコードを提出してきました。

class User < ApplicationRecord
  def self.import_users(emails)
    transaction do
      emails.each do |email|
        User.new(email: email).registration!
      end
    end
  end

  def registration!
    password = PasswordGenerator.gen(email)
    self.password = PasswordGenerator.gen(email)
    save!
    RegistrationMailer.send_mail(email: email, password: password)
  end
end

さて、勘のいい方はお気づきの事と思いますが、このコードには致命的な問題があります。

ロールバックユーザーにメール送っちゃうよ！

全てのメールアドレスが正常処理となった場合には、このコードで何も問題がありません。問題なのは、途中にエラーとなるメールアドレスがあった場合です。

以下の実行例を見てみましょう

emails = %w[
  hoge1@email.com
  hoge2@email.com
  hoge3@email.com
  invalid_mail ### これ失敗する
  hoge4@email.com
  hoge5@email.com
]

User.import_users(emails) rescue puts $!.message

これを実行すると、以下のような出力となります。

トランザクション開始
[hoge1@email.com] をDBに保存しました
[ようこそhoge1@email.comさん。パスワードは2f90b7e4d41753b2です] と送信しました
[hoge2@email.com] をDBに保存しました
[ようこそhoge2@email.comさん。パスワードは54e36f92df5ba1aです] と送信しました
[hoge3@email.com] をDBに保存しました
[ようこそhoge3@email.comさん。パスワードは3f2f7d323f1f0f1eです] と送信しました
ロールバックしました

DBへの登録はロールバックされていますが、hoge1、hoge2、hoge3さんにメールを送信してしまっています。
当然メール送信はロールバックされません。メールを受け取ってしまったhoge1さんたちはログイン画面でパスワードを入力してもログインできることはないでしょう

問題の構造

ユーザー一人分の処理は、「パスワード作成」→「DB登録」→「メール送信」の順で機能を実行しました。ユーザーが複数になる場合には、以下の2通りの方法が考えられます。

パターンA: ユーザー毎に実行する

  • 1人目:「パスワード作成」→「DB登録」→「メール送信」
  • 2人目:「パスワード作成」→「DB登録」→「メール送信」
  • ...
  • n人目:「パスワード作成」→「DB登録」→「メール送信」

パターンB: 機能毎に実行する

  • パスワードの作成:「1人目」→「2人目」→...「n人目」
  • DB登録:「1人目」→「2人目」→...「n人目」
  • メール送信:「1人目」→「2人目」→...「n人目」

パターンBの実装例

新人君はパターンAでやってしまいましたが、ベテランエンジニアなら今回の要件の場合パターンBでやらないといけないことに気がつくと思います。なぜ新人君はパターンAでやってしまったのでしょうか？

お先にパターンBの実装例を見てみましょう

class User < ApplicationRecord
  def self.registration!(emails)
    # メール-パスワード対応表を作成
    email_passwords = emails.map {|email| [email, PasswordGenerator.gen(email)]}

    # ユーザー毎に永続化
    transaction do
      email_passwords.each do |email, password|
        user = new(email: email)
        user.password = password
        user.save!
      end
    end

    # 全ユーザーの永続化に成功してから、全員にメールを送信
    email_passwords.each do |email, password|
      RegistrationMailer.send_mail(email: email, password: password)
    end
  end
end

このコードの実行例は以下のようになります

emails = %w[
  hoge1@email.com
  hoge2@email.com
  hoge3@email.com
  invalid_mail
  hoge4@email.com
  hoge5@email.com
]
User.registration!(emails) rescue puts $!.message

トランザクション開始
[hoge1@email.com] をDBに保存しました
[hoge2@email.com] をDBに保存しました
[hoge3@email.com] をDBに保存しました
ロールバックしました

永続化に失敗したので、メール送信は実行されることがありません。
実は新人君の最初に作った User#registration! は全く使っていないことにも注目です。

なぜ新人君はパターンBでやらないのか？

当然最初に作った実装を活かしたいということは何ら不思議ではありません。
そしてパターンAではループは1回しかないのに、パターンBではループはなんと3回も回っています。

実は新人君にも「効率悪くないですか？」と突っ込まれました。
「そういう物なのだよ坊や( ・`ω・´)ｷﾘｯ」とか「いうこと聞かないと後で酷いぞ（｀ェ´）」などと黙らせてしまうのではなく、新人君の質問には論理的な回答を用意してあげるのが先輩の役割かと思います。

パターンBは本当に効率が悪いのか？

5ユーザーの一括登録に成功する場合に実行した処理数を考えてみましょう。

  • パターンAでは「パスワード作成」「DB登録」「メール送信」の3機能を5回実行します
    • 3機能を5回実行するので、3×5=15処理となります
  • パターンBでは「パスワード作成」を5回、「DB登録」を5回、「メール送信」を5回実行します
    • 5回ずつ3機能を実行するので、5×3=15処理となります

...変わらない！

はい処理数は変わらないんです。もちろん両者の時間計算量オーダーは、ループの実装個数はユーザー数nに依存しないので共にO(n)となります。

メモリ効率は悪くないの？

パターンBでは email_passwords に取り扱うメールアドレスとパスワードの組を全てメモリに乗せています。
パターンBの欠点として、登録数nが1万とか100万の単位になると、メモリを使い切ってしまう恐れがあります。

一方パターンAでは、password 変数が上書きされていくだけです。GCが適切に走るはずなのでOutOfMemoryエラーになる事はないと言って良いでしょう。

しかしながら、本当に「効率」といって領域計算量のみを対象と考えていたのでしょうか？

実は再利用できない物を再利用していなかったか？

パターンAを単純にループ回すだけで済むならとても簡単でした。しかし、パターンAとパターンBではやりたいことは同じですが、やっていることは構造上全然違っていました。

パターンAでは、ループの中で「パスワード発行」「永続化」「メール送信」と3つもの機能を順番に実行していました。実装上これらは密に結合していて単一責任原則に反しており、再利用できなくなってしまっていたと思います。

ループ内の各要素は手続き処理しないといけないか？

さて、実はここからが本題です。長い前置きでした。

新人君に限らず、ループは1回しか書きたくないという欲求は誰にでも(私にも)あるとおもいます。以下の2つのコード例から検討してみましょう。

log_lines.each do |line|
  next if line.start_with?('DEBUG')
  if line.start_with?('FATAL')
    puts "LOOKME: #{line}"
  else
    puts line
  end
end

log_lines
  .reject { |line| line.start_with?('DEBUG') }
  .map { |line| line.start_with?('FATAL') ? "LOOKME: #{line}" : line }
  .each { |line| puts line }

どちらのコードも「DEBUGな行を排除」してから、「FATALな行だけを強調」したものを、「標準出力する」ということをやりたいものです。

前者は各要素を手続きで処理し、後者は集合に対する操作を組み合わせています。集合操作(#reject、#map、#each) に慣れている方なら、前者より後者の方がやりたいことがわかりやすいのではないでしょうか

前者はputsが至る所に登場するので、例えば「FATALを強調してからDBに登録する」というちょっと別のことがやりたくなっても、putsが邪魔をして再利用できない格好になっています。

一方後者は各集合に独立した操作を組み合わせているので、各機能の実装は簡単にメソッドに切り出したりすることができると思います。関数型言語で実装をしている感覚に近いと思います。

まとめ

我々は処理を手続きで考えることが多く、それをそのままそのまま手続きで実装しがちです。
しかしながら、例えば複数ユーザーのリスト、ログファイルの複数行、DBからSELECTしたレコードなどなど我々は集合を取り扱うことがよくあるとおもいます。

せっかく得られた集合からいきなり要素を取り上げて手続き処理していくのではなく、集合をどのように操作・変形・組み合わせることで機能を実現するかの視点に立つと、可読性の高く保守性の高いコードを生産できると私は思っています。

ループ実装数を増やしてもO(n)なのは変わらない。ならば可読性や保守性を取る方がよいのではないか？と思うところです。

おまけ

パターンBをさらにリファクタリングしてみました。各機能を説明していたコメントを全部メソッドに切り出しました。

User.import! は独自実装してますが、activerecord-importを使うとバルクインサートとなるのでパフォーマンス改善が期待できそうです。見通しが良くなったので改善アイデアが生まれやすくなりました

class User < ApplicationRecord
  class << self
    def registration!(emails)
      email_passwords = generate_email_passwords(emails)
      import!(email_passwords)
      send_mail_to(email_passwords)
    end

    private

    def generate_email_passwords(emails)
      emails.map {|email| [email, PasswordGenerator.gen(email)]}
    end

    def import!(email_passwords)
      transaction do
        email_passwords.each do |email, password|
          user = new(email: email)
          user.password = password
          user.save!
        end
      end
    end

    def send_mail_to(email_passwords)
      email_passwords.each do |email, password|
        RegistrationMailer.send_mail(email: email, password: password)
      end
    end
  end
end

⚓参考: 今回の実装例の動作確認用のコード

RailsのAPIを簡易的に実装してみました。

class ApplicationRecord
  attr_accessor :email
  attr_writer :password

  def self.transaction
    puts 'トランザクション開始'
    yield
    puts 'トランザクション完了'
  rescue
    raise 'ロールバックしました'
  end

  def initialize(email:)
    self.email = email
  end

  def password
    'secret'
  end

  def save!
    raise 'DBエラーです' unless email.include?('@')
    puts "[#{email}] をDBに保存しました"
  end
end

class PasswordGenerator
  def self.gen(str)
    str.hash.abs.to_s(16)
  end
end

class RegistrationMailer
  def self.send_mail(email:, password:)
    puts "[ようこそ#{email}さん。パスワードは#{password}です] と送信しました"
  end
end

お便り発掘

関連記事

Ruby: eachよりもmapなどのコレクションを積極的に使おう（社内勉強会）

Ruby: `Hash.new`に渡すデフォルト値の破壊的変更に注意


タグ
この記事を書いた人

kazz

Javaエンジニアで生きてきましたがこのたびRailsエンジニアになれました。 オブジェクト指向中毒者のおっさんです。

kazzの書いた記事一覧へ

本記事の内容へのお問い合せはTwitterで@techrachoへMentionまたはDMにてご連絡頂くか、運営会社であるBPS株式会社のお問い合せフォームよりお問い合せ下さい。

CONTACT

TechRachoでは、パートナーシップをご検討いただける方からの
ご連絡をお待ちしております。ぜひお気軽にご意見・ご相談ください。
CONTACT FORM