僕は6月からWebエンジニアとして自社開発企業で働かせてもらっています。
先輩方から今週いただいたレビュー内容をまとめます。
正規表現が長くなってしまったら要件を疑う
正規表現は長くなるほど実行速度が遅くなり、セキュリティの観点(ReDoS攻撃など)で攻撃されるリスクがあるから、短ければ短いほど良いです。長くなってしまった時点で要件を疑いましょう。
メッセージ内に特定の文言が含まれていたら検知する処理を作成していた際にこの指摘をいただきました。
「なんとか要件を満たす正規表現を作ろう！」と思い試行錯誤の末とても長い正規表現がようやくできたのですが、そもそも「長くなっている時点で何かがおかしい」と思うべきだったんですね、、。
完全に目的を見失ってしまいました。
今後の対策
そもそも正規表現が長くなった時点で疑問を持つ習慣を身につける。
1行程度でパッと作れなければその時点でおかしいと判断し要件の調整を提案すべき。
定数の配列の可読性の高い書き方
私なら後から追加しやすいように以下のように書きますが、いかがでしょうか？
これだけだと分からないためサンプルコードも貼ります。
私は次のように記述していました。
# メッセージ内で使われていたら検知したい単語
NG_WORDS = ['あああ', 'いいい', 'ううう', 'えええ', 'おおお', 'かかか', 'ききき', 'くくく', 'けけけ', 'こここ']
一方でレビューをくださった先輩の提案は以下のとおりです。
# メッセージ内で使われていたら検知したい単語
NG_WORDS = [
'あああ',
'いいい',
'ううう',
'えええ',
'おおお',
'かかか',
'ききき',
'くくく',
'けけけ',
'こここ',
]
たしかに先輩の提案の方が、後から単語が増えた場合に追加するのが簡単ですね。
同様のケースがあったら絶対マネしようと思います。
トランザクションの利用
一つの処理の中で複数のモデルを更新する場合はトランザクションを切るようにしましょう。片方の更新が成功し片方が失敗した場合に不整合が生じます。
僕は次のように実装しておりました。
def perform(user_id:)
# 省略
user.save!
user_auth.save!
# 省略
end
ただしこれだと、user.save!が成功しuser_auth.save!が失敗した場合にデータの不整合が発生してしまう、ということでした。
そのため下記のようにtransactionを用いる必要があります。
def perform(user_id:)
# 省略
User.transaction do
user.save!
user_auth.save!
end
end
上記のようにtransactionを利用すれば、ブロック内の全ての処理が成功した時のみ保存され、いずれか一方が失敗した時はロールバックされるため、データの不整合が生じません。
今後の対応策
同一の処理の中で複数のモデルが更新されていないか確認する。
更新されている場合は必ずtransactionを用いる。
メソッド名がわかりにくい。
setやgetをメソッドの接頭辞に付けるのはJavaっぽくてRubyの慣習に合わないので命名しない方が良いです
僕は次のように実装していました。
def perform(user_id:)
user = User.find(user_id)
user_auth = user.user_auth
# このメソッドの命名が良くない
set_user_auth(user_auth)
# ここから先は省略
end
private
# このメソッドの命名が良くない
def set_user_auth(user_auth)
user_auth.inactivated_flag = true
user_auth.restrict_user_flag = true
end
上記のset_user_authという命名がRubyっぽくないし、何を表しているのか分からない、という内容でした。
実は僕は前職でJavaを触っていたため、その名残でこのような書き方をしてしまいました、、。
なのでレビューにあった「Javaっぽい」という指摘はごもっともですね。
指摘を受けて次のように修正しOKをもらいました。
def perform(user_id:)
user = User.find(user_id)
user_auth = user.user_auth
# この方が分かりやすい
freeze_user_account(user_auth)
# ここから先は省略
end
private
# この方が分かりやすい
def freeze_user_account(user_auth)
user_auth.inactivated_flag = true
user_auth.restrict_user_flag = true
end
今後の対応策
set,getを接頭辞に使わない。
メソッド名を考えるときは、そのメソッドが何をしようとしているのか明確な命名にする
最後に
同じミスは二度としない！と決意しているつもりですが、それでもしてしまうのが人間です。
一度もらったミスを繰り返さないよう定期的に見返します