気ままに気ままのエンジニアブログ

定期的に得た知見を気ままに発信中

1年前に投稿した「公開コードレビュー会に参加してみての感想」を、1年後の自分がレビューしてみた

こんにちは。

不意にブログ名を変更してみました。

どうもハチマキです。

はじめに

今から1年ちょっと前に、未経験からエンジニアに転職致しました。当時はよく初学者向けの勉強会に参加して、そこで得た学びを記事に書いていました。

現在エンジニアになって約1年半、前提知識や経験は、当時から考え難いくらい成長してきたなと我ながら感じております。

この1年でどのようなことを感じるようになって、どのような視点を持つようになったのか、自分自身の成長加減が気になったこともあり、当時書いた勉強会の記事をベースにあたかもコードレビューかのように、コメントを加えていきながら振り返りを行ってみたいと思います。

▼この記事の内容にレビューを加えていきます
hachimaki37.hatenablog.com

対象

  • 実際エンジニア歴1年ってどんな感じなのかを知りたい
  • プログラミング挫折しそうな人
  • エンジニア成り立てってどんな感じなのかを知りたい

環境

当時参加した勉強会の概要

  • レビュー会概要
    • ポートフォリオレビュー者(3名)に対して、一人30分程度で現役エンジニアの方が公開レビューしていく
    • レビュー会のみ参加者:15名ほど(私はこちらで参加)
    • 平均年齢:おそらく20代中盤
  • 3名のポートフォリオ一覧(個人アプリケーション)
    • 1人目:昆虫食専門のサイト
    • 2人目:一人旅行で現地で遊んでくれる人を探すような掲示板サイト
    • 3人目:本のアウトプットアプリ

では、早速いきましょう!

総論に対するコメント

1番の学びポイントは、自身のマインド面に変化をもたらした。
同じような駆け出しエンジニアの人たちであろう、自身の作成したサービス(ポートフォリオ)に素直に驚かされた。

と同時に、レビューされる人はもちろん、レビュー会のみ参加している方々の温度感、熱量に焦りすら感じた。
自身を見直し、改めて計画して頑張ろってなった。

今振り返っても、この勉強会で得たマインドの変化は、良い緊張感と良い焦りを与えてくれた機会でした。

発表していたポートフォリオは、今思えばシンプルな作りではあるが、当時の自分には恐ろしいほどのレベルの差を感じました。
そこから得た学びで、目標や計画に落とすようになり、実行に移せたからこそ今の自分があるように感じます。
LGTM👍

テーブル名やカラム名、メソッド名の一つ一つに意図した意味をしっかり持たせながら設計していくことの重要性を学んだ。
ちゃんと一つ一つに開発者の意味がこめられていることを知らなかった。

メソッド名から変数名など、一つ一つの意図を考えながら実装していく必要があります。
意図した意味というよりも、そのコードを見た他者が迷いなく、意図を理解できるような設計にすることが重要です。

*保守のしやすさ(仕様理解や開発効率)に直結するため

学んだことに対するコメント

・レビューする側の視点を知れたこと
 ・現在、レビューしていただく側のため、レビューする側の観点や流れをざっくり知ることができた

・具体的に
 ・メソッド名 / テーブル名 / カラム名は、意図した(相手へのメッセージ)名称の定義を行なっているか
 ・テーブル名、カラム名が、UI画面と紐づくように意図して定義されているか
 ・コードに処理の意図を感じ取れるか
 ・コードは、シンプルにわかりやすく書かれているか

プログラムの可読性や開発効率を上げるための重要な観点だと感じます。
今はまだレビューして頂く側ではあるが、コードレビューのコメントで上記箇所は、良し悪しに関わらず必ずコメントを頂いているように感じます。

メソッド名 / テーブル名 / カラム名は、意図した(相手へのメッセージ)名称の定義を行なっているか

上記にも追記しましたが、保守性などに起因する重要なポイントです。

テーブル名、カラム名が、UI画面と紐づくように意図して定義されているか

どのようなデータを格納するためのカラムなのかを考えた上で、命名する必要があります。チーム内でも命名に関して相談をする機会が多々あります。

コードに処理の意図を感じ取れるか

自分にしか理解できないコード(スパゲティコード)になっていないかは重要な観点です。
スパゲティコードになることで、保守性が下がる。つまり開発効率が下がることに直結する点ですので、気をつける必要があります。

例) 
#バッドコード
@users = User.find(1) 

#グッドコード
@user = User.find(1)

#バッドコードの理由:ユーザidを取得しているのに、変数名が複数形になっている

コードは、シンプルにわかりやすく書かれているか

可読性を上げるために、不要(余計)なコードは書かないこと、コード短縮できる箇所は、短縮したコードを書くこと。

例)
#バッドコード
user = User.find(1)
user.id = 10
user.save

#グッドコード
user = User.find(1)
user.update(id: 10)

#バッドコードの理由:短縮できるコードは、リファクタリングしましょう。

・レビューする時のfile確認の流れ
 ・README(github) → schema → routes → models → controllers → ・・・

レビュー観点という面では、アプリケーションの全体像(README(github) → schema..)から具体(controllers →..)に確認を進めていくことで、スムーズなレビューにつながるように感じました。(まだレビュアーではありませんが・・)

各fileの確認ポイントに対するコメント

アプリケーション概要を把握するためのREADMEに詳しく書く
概要
技術ポイント
機能一覧
環境 / 技術構成
インフラ構成
テーブル設計
etc...
 ※インフラ構成や、テーブル設計を記載する際は、図で表現するとイメージがついて良い

アプリケーションの概要を理解するために、READMEを詳しく書いておくことは良い点かと思います。
READMEを見ることで、他社がアプリの全体像を掴めること=スムーズなコードレビューに繋がる要素かと思います。

schemaに対するコメント

migration file内に、null: falseを書く癖をつける

データの整合性を保つために必要です。null: falseを指定したカラムは、空の状態でDB保存できなくなるため、例えば、フォームの氏名を必須にした時に、null: falseを定義しないと、氏名は未記入のままDBに保存されてしまいます(データが不整合になる)

すると、思わぬバグになったりするので、必要な箇所にはnull: falseを追加するようにしましょう。

テーブル内で重複するデータを禁止するために、unique: trueを書く

unique: trueは、重複レコードを1つにまとめるためのメソッドです。
例えば、userモデルのemailカラムに、a@gmail.com、b@gamil.com、a@gmail.comのメールアドレスがあった場合、a@gmail.comの重複データをまとめてくれます。

定義する場合は、下記のようにindexにuniqueを貼る必要があります。

~~省略
add_index  :users, [:email], unique: true

*補足

  • uniqueメソッド:Rails5以降で非推奨
  • distinctメソッド:Rails5以降で正式メソッド

Rails 一意性制約のかけ方|Nori|note
Railsでreferencesを使用した外部キーに、同時にUnique属性を設定する - TechBox

命名に不可算名詞を使うことは、良し

理解に戸惑う可能性があるなら、出来るだけ不可算名詞は避けた方が良い。
不可算名詞の場合、rails g modelを実行した時にrails側で識別できず、余計なsやesがついてしまう恐れがあります。(config/initializers/inlrections.rbをいじれば問題は解消できる)
Railsの命名規則【単数形・複数形】 - 箱のプログラミング日記。

テーブルの紐付けは、リレーションシップをうまく使い分ける

テーブル同士の関連付けを、各モデルに定義(belongs_to、has_many)する事で、直感的なコードでデータ取得が可能になります。

#user.rb 
belongs_to :user

#schedule_result.rb
has_many :schedule_results

#レコード取得する時
% user.last.schedule_results
% schedule_results.last.user

【Rails】アソシエーションを図解形式で徹底的に理解しよう! | Pikawaka - ピカ1わかりやすいプログラミング用語サイト

DBのテーブル名、カラム名をユーザー行動から考えて作成する

ユーザ操作に関連した、DB設計が重要です(ユーザ操作と全く違うデータを管理するのはナンセンスです)
また命名規則がいくつかあるため、確認しながら進めていきましょう。
Railsにおける命名規則 - Qiita
データベースオブジェクトの命名規約 - Qiita

テーブル数を意識して、作成する

テーブル数よりも、1テーブルあたりのカラム数を意識したほうが良いです。
とあるデータでは、1テーブルが持つべきカラム数は、10以下が妥当とのこと。10カラムを超える設計であれば、正規化できないか一度見直す必要があります。
先人達から学ぶRailsのテーブル設計 - Qiita

routesに対するコメント

ユーザーにわかりやすく、意図したURL設計をしていく

ユーザにわかりやすくというよりも、何に対して (URL)/処理内容に適した (HTTP Method)URL設計にすることが重要です。
*覚えやすく、どんな機能を持つURLなのかがわかるようにする
リソースの一部更新におけるURL設計 - Qiita
RESTful APIのURI設計(エンドポイント設計) - Qiita

resourcesメソッドを使ったroutes設定

RESTの原則に則って構築しましょう。resourcesを定義することで、railsですでに定義されている7つのアクションのHTTPメソッドを活用したroutes設定ができます。

*GET, POST, PUT, PATCH, DELETE → index, new, show, create, update, destoryアクションに対応
resources | Railsドキュメント
【Rails】resourcesメソッドを使ってルーティングを定義しよ | Pikawaka - ピカ1わかりやすいプログラミング用語サイト

member routesを活用する → 初学者の場合、get / postのみで書いてしまうことがある

上記で書いた7つのアクション以外にもアクションを定義したい時に有効です。memberは、特定のデータ(users/:id/likesのように:idが入るアクション)に対するアクションを定義することができます。
railsのroutes.rbのmemberとcollectionの違いをわかりやすく解説してみた。〜rails初心者から中級者へ〜 - Qiita

URLに動詞を入れない(規約で決められている)

動詞ではなく、名詞を活用すること。
*動詞を使用することで、変数や関連を使う場合に不自然な命名になってしまうため
モデルやメソッドに名前を付けるときは英語の品詞に気をつけよう - Qiita

単数形リソースを使った方がわかりやすくなります。

idが不要な場合は、単数形リソース(resource)を活用した方が、シンプルでわかりやすくなる
*例えば、/profile/:idのようなパスを、/profileで割り当てることができます。
Rails のルーティング - Railsガイド

ホワイトリスト形式とブラックリスト形式の使い分け

形式を分けることによるセキュリティ対策です。

  • ホワイトリスト形式
    • 「安全な対象」を定義したリストです。リストで定義されているアプリケーションだけを実行させ、リストに定義されていないアプリケーションは実行できません。安全なものだけを利用し、それ以外はすべてブロックするために存在するリストです。
  • ブラックリスト形式
    • 警戒する必要がある「危険な対象」を定義したリストです。リストに定義されているアプリケーションは実行させません。使用してはいけない危険なアプリケーションやプログラムを定義し、危険だと分かっているものを利用しないために存在するリストです。

ホワイトリスト式セキュリティの仕組みとは?ブラックリストとの違い | DAiKO+PLUS(プラス)
ホワイトリスト方式セキュリティの仕組みと効果:株式会社 日立ソリューションズ・クリエイト

shallowオプションでroutesを整える

長くなるURL問題を解消してくれるオプションで、最小限の情報でリソースを一意に指定できるルーティングを作成できます。
ただ、デメリットもありそうなため、使用する際には、改めてよく調べてから使いましょう。
Railsのroutesのshallowは安易に使わないで欲しい – Matsubo Tech Blog
uRails のルーティング - Railsガイド

modelsに対するコメント

最大文字数が必要な箇所には、バリデーション(lengthオプション)を定義してあげる

ユーザビリティを高める一つの手段として、良いかと思います。文字数に制限を設けることで、最小文字数や最大文字数を設定することができます。
例えば、最大文字数を超えている場合や最小文字数に達しない場合にエラーを表示する事で、ユーザアクションの迷いが軽減されます。

#user.rb
validates :name, length: { maximum: 10, message: '氏名は10文字以下に設定して下さい' }

バリデーション例外を捕捉する "!"を加える

エラー原因の調査に役立ちます。
「!」の有無の違いは、保存出来なかった場合の振る舞いに違いがあります。
エラー内容を知りたい場合は「!」を追記し、例外を発生させる事でその内容に応じたエラー文を表示させましょう。

例)

if @user.save
# 保存が正常に完了すれば「true」、保存できない場合「false」を返します。

if @user.save!
# 保存が正常に完了すれば「true」、保存できない場合、例外ActiveRecord::RecordInvalidが発生します。

rails save! create! update!のバリデーション例外を捕捉する - Qiita

他のユーザーが編集不可能にするために、current_userと紐づける

current_userを使用することで、サインインしているユーザーを取得することができます。
例えば、current_userに紐付けず実装した場合、他のユーザが編集や削除などが出来てしまいます。そのため、current_userを用いてセキュアなアプリケーション構築を心がけましょう。

Active Modelを使用し、簡潔に書いていく

多くのモジュールを含むライブラリを用いることで、シンプルかつ簡単に実装が可能になります。
Active Model の基礎 - Railsガイド

複雑な処理は、controllersに記述していくのではなく、modelsにメソッドを用いて記述する

ロジックをControllerに記載することも可能ですが、ファットコントローラーになり、コード管理がしづらい状態になります。
そもそものControllerの役割は、ViewとModelを繋ぐ(ユーザーからの要求を処理し、モデルやビューと連携を行なう)ことです。データの表示はViewに、ロジックの実行はModelに切り分け、MVCアーキテクチャーをうまく活用した実装にしましょう。
【Rails】MVCフレームワークを初心者向けに丁寧に解説! | Pikawaka - ピカ1わかりやすいプログラミング用語サイト

スコープを積極的に使用し、意味のあるまとまりを作る

Active Recordの機能の一部で、モデルにscopeを定義することにより、複数のクエリを一つのメソッドにまとめることができます。
コントローラーで複数のクエリを書くよりもscopeを使えば、コードがスッキリする。

例)

scope :active, -> { where(is_active: true) }
default_scope  -> { user(name: "DESC") }

Railsのモデルのscopeを理解しよう - Qiita

has_manyを使って、追加されるメソッドを使う
アソシエーションを貼ることで、使えるメソッドが複数ある

has_manyを定義することで、class_name、source、foreign_keyなどのオプションが使えるようになるため、うまく活用しながら効率的に実装を進めていきましょう。
railsアソシエーションオプションのメモ - Qiita
has_many | Railsドキュメント
【Rails】Associationメソッドまとめ - Qiita

findとfind_byの使用を明確にする

  • find
    • idの値が分かっていて、そのidのデータを取得したい場合に使用する

*各モデルのidを検索キーとしてデータを取得するメソッド
*id以外の条件で検索不可

例)

User.find(1) 
  • find_by
    • idの値が不明で、id以外のカラムを検索条件としたい場合に使用する

*各モデルをid以外の条件で検索するメソッド(idでも検索可能)
*複数の検索条件を指定可能
*返ってくる結果は、最初にヒットした1件のみ

例)

User.find_by_email('test@gmail.com') 

find、find_by、whereの違い - Qiita

該当するレコードがあれば取得、無ければ作成してくれるfind_or_create_byメソッド

引数の条件に該当するレコードがあればそれを返し、もしなければ新規作成するメソッドです。

例)

User.find_or_create_by_name('テストベッカム')

controllerに対するコメント

controllersでの処理はシンプルに記述する
modelsに処理を切り分ける

上記同様のコメントを参照。
ロジックをControllerに記載することも可能ですが、ファットコントローラーになることで、コード管理がしづらい状態になるため、ロジックはmodelsに切り分けて書くようにしましょう。

最後に

これでレビューは以上になります。

当日は、〇〇は〇〇だ!というように、あまり細かな疑問を持つことはなかったように感じます。前提知識が徐々に積み上がってきたことで、このコードはこうした方がより良いのでは?などの疑問や考えが出てきたように思います。

まだまだ技術面はジュニアですが、スキル向上に向けて、引き続きコツコツと努力していきたいと思います。

*実は今回の記事レビューを思い立った理由は、チェリー本(Ruby)を読んだアウトプットでした。
ただ以前書いた記事の内容は、railsがメインだったなぁと思ったので、改めて別のアウトプットを考えてみようと思います。

                                            • -

日々勉強です。
以上、ハチマキでした。