外部講師CTOの方のコードレビューまとめ

Ruby on Railsアイキャッチ画像
2021.07.21

外部講師の、ある企業のCTOの方のコードレビューが開催されたためその内容についてまとめました。

正直、今まで開かれた催し物の中で最も勉強になった。

各ファイル毎に段落を区切ってまとめています。

目次
  1. アプリの確認
  2. コードの確認
  3. 採用時に見るポイント
  4. まとめ

アプリの確認

  • 検証画面で確認し、javascriptのエラーが出ていないか
  • ユーザーが使用を離脱してしまうデザインとなっていないか
    バリデーションエラー発生時に登録した内容が消えてしまわないかなど

コードの確認

README

  • pry-railsなどのデバッグ用gemはREADMEに書かない！

そんなところまで違和感を感じるんだ！！

  • テーブル定義で、文字数を指定する場合は根拠を記述すること
  • 要件定義の5つの資料（カタログ設計、テーブル設計、ER図、画面遷移図、ワイヤーフレーム）は整合性を持たせること

これらの資料は結構ちゃんと見られている

  • READMEにデプロイ先URLを載せる場合は、IPアドレスでなくドメイン名とした方が良い
  • URLカラムのデータ型はtextでなく、stringが適切

gem

  • 開発環境でしか使わないgemはdevelopmentに書く。Rubocopなど

route

  • resourcesのonlyをちゃんと使う。onlyに指定するアクションがいっぱいある場合は、逆にexceptを使う
    https://railsguides.jp/routing.html
  • 以下のようなルートを作る場合、敢えてtopにネストする（collectionを使う）必要は無い。
  resources :top, only: %i[index] do
    collection do
      get :user_guide
    end
  end

model

  • ロジックをscopeを使ってmodelに移す場合、以下のような、むしろ可読性が悪くなるようなものは書く必要はない
scope :order_record_on, -> { order(record_on: :desc) }
scope :order_id, -> { order(id: :desc) }

controller

  • 以下のようなeach文はmapメソッドを使えば一行で書ける
      @bmis.each do |bmi|
        @chart_elements << [bmi.record_on, bmi.status]
      end

mapを使うと

@chart_elements = @bmis.map{|bmi| [bmi.record_on, bmi.status] }

  • allの使い方には気をつける。N+1問題
    このような使い方はよくない。
  def index
    @foods = Food.all
    @foods = @foods.pick_current_user_id(current_user.id).order_id
    @foods = Kaminari.paginate_array(@foods).page(params[:page]).per(10)
  end

逆に.allを使用しても問題ない例

Post.allのようにユーザーが登録するデータに対し.allとすると、取得するデータ料が肥大化するけど、categolyのように登録で増えていかないものに対しては使用しても問題ない。

  • before_actionの使用について
    before_actionが複数になってくるとわけが分からなくなってくる。
    これを使うのは、deviseのbefore_action :authenticate_user!といった、権限を与える、controller全体に影響を与えるものにのみ限定して使用するべき。
before_action :set_food, only: %i[show edit update destroy]
  • ifにifがネストされるような書き方になることがないようにする
  • findでデータ取得してから=で確認するような書き方は冗長
    例えば以下の例では、以下のように書き換えできる。
@post = Post.find(params[:id])
unless current_user == @post.user
  redirect_to user_path(current_user)
end

以下のように書き換えできる。

@post = current_user.posts.where(@arams[:id]).first
redirect_to user_path(current_user) if @post.blank?

採用時に見るポイント

  • 成長角度
    角度が大きいことのアピールが必要
  • 努力と思考ができる人か
  • 一番重要視するのは自走力。自走力の定義は２つ
    -与えられた課題を自力で解決する力
    -課題を与えられずとも発見する力
  • 内省習慣
    経験→振り返り考える→概念化→思考
    このサイクルができる人か
  • 前職で苦しかったことは何か？という問いに対し、
    “他責”にする人は一発アウト！
  • 教育コストがかからない人か
    自分の中で学習方法の型がある人なのかを見ている。
  • モチベーションは何かという抽象的な質問に対し、
    考えること、言語化ができる人であるかを見ている。
  • どんなエンジニアと働きたいか
    「優秀な人」と答える人が8割くらいだそう。
    そう答える人はつまり、教えてもらおうという他責の姿勢である。
    それでその人の価値観が分かる。
  • コミュニケーションがうまいなと思う人
    チャットでのコミュニケーションがメインであるため、状況や、思いを的確に言語化でき、まめな人

まとめ

驚いたことがたくさんありました。

returnの使用や、ルーティングのcollection、コントローラーのbefore_actionなど、これまで教科書通りのコーディングをしてきたつもりでしたが、rubyでもreturnを使用したり、DRY原則を少し無視することもあることを学びました。ルールに固執することなく、目的や場面によって使い分けることの重要性を知ることができました。

コメント