はじめに
こんにちは、株式会社COUNTERWORKSのくりまろ(@kuririmaro)です。
約1週間ぶりの登場です。
前回の記事も読んでくださいましたでしょうか？便利なツールをまとめているので、ぜひご覧下さい！
社会人になって9ヶ月も経ってしまいました。
どの企業の、どの部署の新入社員も 先輩達からの無理難題 様々なタスクを消化してきたかと思います。
かくいう私も数多くのタスクをこの9ヶ月の間に消化してきました！
まずはここまで辞めずに仕事を続けている全国の新入社員に拍手👏
今回の内容
Go Forward
長期的に社会にインパクトを与える大きな事業を作り出すには、ビジョンの実現に向けて、自らが考え実行し続けることが必要です。スピード感を持って多くのトライアル&エラーを繰り返すことで最大の成果を出し、事業を前進させます。
One Team
事業を継続的に大きく前進させるには、すべてのメンバーの力を合わせることが重要です。自らのミッションに対してオーナーシップを持って仕事に取り組むことで、チームのパフォーマンスが最大化されます。
また、すべてのメンバーがプロフェッショナルとして、能力を高め行動し続けることによって、チームの力をより大きく成長させます。
Keep Straight
マーケットを代表する企業であり続けるために、誠実で利他的な行動によってすべてのステークホルダーの長期的な支持と信頼に応え、共栄を目指します。
この３つは弊社のvalueです。社員としてはこの３つを達成していかなければなりません。
日々目の前のタスクを消化する毎日だと、自分がこのvalueを達成出来ているのか、もといどれだけ技術に対しての知識が付いて、技術的に成長出来ているのかを実感できないですよね？
なので、今回は新卒研修で作ったwebアプリのバックエンドのコードをレビューして見て、どれだけGo Forwardできているのか確認してみましょう。
過去のコードを見て、今の私ならこう書くのに！！とリファクタリングも実施します。
当時も先輩からレビューいただいているので、PRを遡って、レビューを出す前のコードを対象にして、見ていきます！
作ったwebアプリの概要
ざっくりまとめると
- Ruby on RailsとTailwind CSSで作成
- CRUD処理をすべて使うアプリケーション
- 簡易的な記事投稿サイト
- コメントも付けられる
- ログインはGoogleログインを使う
です。
2月-4月までの3ヶ月で設計からデプロイまで行ったので、初学者の私には作り込んだものは作れませんでした。
レビューしていく
全てのコードをレビューしている時間はないので、記事のCRUD処理に関する箇所だけレビューしていきます！
create
def create
@article = current_user.articles.new(article_params)
if @article.save
redirect_to articles_path, notice: '記事を作成しました。'
else
render :new, status: :unprocessable_entity
end
end
private
def article_params
params.require(:article).permit(:title, :content, images: [])
end
指摘点は、データの操作はモデルに寄せましょうということですね。
今のままでは、コントローラがモデルのカラムを知っている必要があるので、凝集度が高いとは言えません。
まずそこを変更したいですね。
気になるのは、createをnewとsaveで分けているところです。
当時は、作成に成功したらって分岐を作るのに必死で、特に気にしていなかったのですが、今は気になるようになっています。
saveが成功したら〜〜〜失敗したら〜〜〜でなく、create!で例外が出たらrender :new, status: :unprocessable_entityをするようにすればいいと思います。
結局createもbuildとsaveをしているので実際はsaveが成功したかどうかを確認しているのにはかわりありません。
以上の２つ点を意識した上でコードを変更していきます！
変更したコードがこちらです。
def make!(title:, content:, images:)
create!(title:,content:, images:)
end
def create
current_user.articles.make!(title: article_params[:title], content: article_params[:content], images: article_params[:images])
redirect_to articles_path, notice: '記事を作成しました。'
rescue ActiveRecord::RecordInvalid
render :new, status: :unprocessable_entity
end
private
def article_params
params.require(:article).permit(:title, :content, images: [])
end
こうすることで、データを作るのはモデル側に寄せることができ、コントローラはデータを渡すだけで、どんなカラムがArticleモデルにあるのかを知る必要がありません。
make!で渡しているのはあくまでも引数なので、カラムの情報ではないことに注意してください！
（local環境がなぜか動かないので、この変更が正しいのか不明です）
read(show)
def show
@article = Article.find(params[:id])
end
うん。
まぁshowメソッドは比較的簡単にできますよね。特に突っ込むところはないですね。。。
強いていうなら、パラメータのidに文字列が入ってきた時でもActiveRecord::RecordNotFoundが返されてしまうので、簡単なバリーデーションを追加した方が良いですね。
privateメソッドにバリデーションをするメソッドを追加して、before_actionで実行するのが簡単そうです。
before_action :validate_id, only: [:show]
def show
@article = Article.find(params[:id])
end
private
def validate_id
unless params[:id].match?(/\A\d+\z/)
render json: { error: "Invalid ID" }, status: :unprocessable_entity
end
end
こうすることでidが間違えているのか、パラメータの入力値が間違っているのかを確認することができます。
update
def update
@article = Article.find(params[:id])
# 添付画像を個別に削除
params[:article][:image_ids]&.each do |image_id|
image = @article.images.find(image_id)
image.purge
end
if @article.update(article_params)
redirect_to @article, notice: '記事を編集しました。'
else
render :new, status: :unprocessable_entity
end
end
image = @article.images.find(image_id)でN+1が起きそうですね。
そしてコントローラーで画像の削除を行っているので、そういったものをモデルに寄せていきましょう。
さて変更しました。それがこちら！
def modify!(title:, content:, images:)
transaction do
images.each(&:purge)
update!(title:,content:, images:)
end
end
def update
@article = Article.find(params[:id])
@article.modify!(title: article_params[:title], content: article_params[:content], images: article_params[:images])
redirect_to @article, notice: '記事を編集しました。'
rescue ActiveRecord::RecordInvalid
render :new, status: :unprocessable_entity
end
private
def article_params
params.require(:article).permit(:title, :content, images: [])
end
今の私ならこうしますかねぇ。
画像データが入ってるテーブルのidから探すのではなく、編集したいデータに関連のある画像データを削除するようにすれば、N+1問題は発生しないはずです。
当時の私との違いはトランザクションを使うようになった点ですね。
transaction do
images.each(&:purge)
update!(title:,content:, images:)
end
このようにトランザクションで囲うことで、updateが失敗したとしても画像が消えたままになることを防いでます。
コントローラに関しては、crateとほとんど変わりありません。
（images.each(&:purge)という記法で動くかどうかは未検証です。もしかしたら動かないかもしれません。）
delete
def destroy
@article = Article.find(params[:id])
if @article.destroy
redirect_to articles_path, notice: '記事を削除しました。'
else
redirect_to articles_path, alert: '記事削除に失敗しました。'
end
end
特にいうことことなしですかね。
destroyがfalseを返す時があるのかわかりませんが、動きそうですね。
感想
過去の自分が書いたコードを読むのは、とても恥ずかしい行為でした。。。。
今まで意識的に避けてきた道でしたが、今回アドベントカレンダーのネタとして実行できてよかったです！
当時はこんなことを考えていたけど、それを考える必要がないのでは？と過去の自分に突っ込んでみたり、今ではこう書けるけど、あの時には知らなかったもなぁと回想してみたりと、単なるレビュー（リファクタ）ではなく、自分の思考の変化や視点の変化など成長を感じるものになりました。
こうしてみると、当時から忙しそうにしていたのにレビューをしてくれていた上司には感謝ですね。
こんなコードをレビューしてくださいって言われたら、私だったら ぶん殴 頭を悩ませてしまいます。
そんな優しい先輩を助けられるように、これからもMAX成長していこうと思いました。
最後に
みなさまから見て私の9ヶ月の成長はどのように映りましたでしょうか？
「この子はすごい！」と思ってくれる方もいれば、「まだまだだね」と越前リョーマばりに煽ってくる方もいらっしゃると思います。
ですが、少なくとも我々新卒は前進できていると思います。それが小さな歩幅で少しずつであったり、大股で一気にであったりするかもしれません。
日々の業務の中で成長を感じることはなかなか難しいかもしれませんが、入社したてのころと9ヶ月社会人をやった今、見比べた時に明らかな差があると思います。時にはがむしゃらに前に進むのではなく、今まで通ってきた道を振り返る時間を取ってみてはいかがでしょうか？
最後までご覧いただきありがとうございました！
今回の記事を読んで COUNTERWORKS に興味を持った方がいらっしゃったら、ぜひ以下のリンクよりご応募ください！
ポップアップストアや催事イベント向けの商業スペースを簡単に予約できる「SHOPCOUNTER」と商業施設向けリーシングDXシステム「SHOPCOUNTER Enterprise」を運営しています。エンジニア採用強化中ですので、興味ある方はお気軽にご連絡ください！ counterworks.co.jp/
Discussion