💛

リーダブルコードの重要性

2023/05/31に公開

Rails

リーダブルコード

tech

はじめに

今日はチーム開発の振り返り日でした！

これからの改善点がめちゃ見つかりましたし、
チームにおいて自分がどのように役に立てていたのかや、
他の人の視点から制作物を見た評価を聞けて、良い意見が多くとても嬉しかったです！

そして、違うチームの受講生の方にチーム開発のコードレビューをもらって、
とーーても勉強になったのでこれからの教訓として残しておこうと思います💪🏻

三項演算子

下記の記述について！

もし is_deleted が true の場合、"退会" を返し、そうでない場合（つまり is_deleted が false の場合）、"有効" を返します。

しかし、Rubyにおいては is_deleted 自体が boolean 値（真または偽）を返すので、それを明示的に true と比較する必要はない！！

app/models/customer.rb

  def customer_status
    if is_deleted == true
      "退会"
    else
      "有効"
    end
  end

よって
if is_deleted == true
ここの部分は
if is_deleted
に省略できる！！

あと、この記述自体も3項演算子を使って以下のようにコンパクトにすることができる！！🥹

app/models/customer.rb

def customer_status
  is_deleted ? "退会" : "有効"
end

この記述どっかで見たなあという記憶はありますが、
コードをどう省略できるのか？までは考えられていなかったので、
教えてもらってめちゃタメになりました！！

でも、リーダブルコードによると
短く書くことは大切だが、短く書くことによって他の人が読んだ時に理解に苦しむようなコードでは本末転倒になってしまう。とのことなので、ほどほどにって感じですかね🤔

一個一個のコードにこれ省略してわかりやすく出来るんじゃないか？って意識持って
調べていく姿勢はめちゃ大事ですね。

参考にさせていただいた記事🌱

メンテナンス性

下記の記述により、
item_with_tax_priceとitem.with_tax_priceの表記揺れが起きていて
価格変更時に漏れが発生する可能性がある。

また、メソッドの役割についてもitem.with_tax_price を直接呼び出しているだけで、
何か特別な処理を行っているわけではないので、混乱を招く可能性もある。
このメソッドが本当に必要な場合、その目的や役割をコメント等で説明した方が良い。

app/models/cart_item.rb

class CartItem < ApplicationRecord
  def item_with_tax_price
    item.with_tax_price
  end
end

可読性を高める（case文）

自分のチームの記述

各パラメータが何を示しているのか、それがどのように使われるのかが一見してわかりづらく可読性が低い。
エラーハンドリングが一部しか行われておらず、具体的なフィードバックが提供されていない場合がある。
同じ処理が複数箇所に分散して書かれている。
複雑であるため、メンテナンスが難しくなる可能性がある。

app/controllers/public/orders_controller.rb

  def confirm
    if params[:order]
    @order = Order.new(order_params)
    @order.customer_id = current_customer.id
    @cart_items = current_customer.cart_items
    @total_amount = @cart_items.inject(0) { |sum, item| sum + item.subtotal }
    @order.postage = 800
    @order_total_amount = @total_amount + @order.postage.to_i

    if params[:order][:select_address] == "0"
      @order.shipping_post_code = current_customer.post_code
      @order.shipping_address = current_customer.address
      @order.shipping_name = current_customer.last_name + current_customer.first_name
    elsif params[:order][:select_address] == "1"
      if ShippingAddress.exists?(id: params[:order][:address_id])
        @address = ShippingAddress.find(params[:order][:address_id])
        @order.shipping_name = @address.name
        @order.shipping_post_code = @address.postal_code
        @order.shipping_address = @address.address
      else
        flash[:notice] = "配送先情報がありません"
        render 'new'
      end
    elsif params[:order][:select_address] == "2"
      @order.shipping_name = params[:order][:shipping_name]
      @order.shipping_post_code = params[:order][:shipping_post_code]
      @order.shipping_address = params[:order][:shipping_address]
    else
      render 'new'
    end
      @address = "〒" + @order.shipping_post_code + @order.shipping_address
    end
end

お手本にしたいチームのコード

可読性が高い。case文を使用して条件分岐を明確にし、パラメータに対するエラーハンドリング（空文字のチェックなど）が直感的に行われている！
DRY原則。同様の処理を複数回書くのではなく、一箇所にまとめている！

app/controllers/public/orders_controller.rb

  def confirm
    @order = Order.new
    @cart_items = current_customer.cart_items
    @payment_method = params[:payment_method]
    @total = 0
    @shipping_cost = 800
    case  params[:address_option]
    when "1"
      @postal_code = current_customer.postal_code
      @address = current_customer.address
      @name = current_customer.full_name
    when "2"
      # 選択チェック
      unless params[:address_id].present?
        flash.now[:danger] = "住所を選択してください。"
        render :new
      else
        address = Address.find(params[:address_id])
        @postal_code = address.postal_code
        @address = address.address
        @name = address.name
      end
    else
      # 入力チェック
      if params[:postal_code]=="" || params[:address]=="" || params[:name]==""
        flash.now[:danger] = "お届け先郵便番号・住所・宛名を入力してください。"
        render :new
      else
        @postal_code = params[:postal_code]
        @address = params[:address]
        @name = params[:name]
      end
    end
  end

読みやすさが一目瞭然ですよね🥺

customerステータスのmodelメソッド化

<td><%= @customer.status %></td> で以下のmethodを呼び出せるようにできる！

def status
    if is_deleated
      ApplicationController.helpers.tag.span "退会", class: "font-weight-bold text-secondary"
    else
     ApplicationController.helpers.tag.span "有効", class: "font-weight-bold text-success"
    end
end

関数から早く返す

関数で複数のreturnを利用することは悪いことではない。むしろ関数から早く返した方が良い。

下記のコードを例として考えてみると、
order.keyやorder_detail.keyが特定の条件を満たさない場合に、早期にリターンしています。
これにより、不適切な状況をすばやくキャッチし、関数の残りの部分を実行せずに済んでいます！

例

if order.key == 0
  flash[:danger] = "注文の入金確認ができておりません。"
  redirect_to admin_order_path(order.id)
  return
end
if order_detail.key >= key
  flash[:danger] = "製作ステータスは変更できません。"
  redirect_to admin_order_path(order.id)
  return
end

早期リターン
特定の条件が満たされない場合、早期に関数から返すことで後続のコードの実行を防ぎ、リソースを節約します。
コードの読みやすさ
ネストされたifステートメントはコードの可読性を低下させる可能性があります。早期リターンを利用すると、深いネストを避け、コードを読みやすくすることができます。
エラーハンドリング
不適切な引数や予期せぬ状況に対するエラーハンドリングを簡潔に書くことができます。これにより、関数が意図した通りの動作をしない場合に早期に返すことができます。

Search機能のView作らんくてもよかった！

わたしは商品名検索後に飛ぶビューを別で作ってましたが、
コントローラーで下記のように記述するだけでもいけるらしいです！

これはあとで実際に試しにやってみようと思います！

app/controller/public/items_controller.rb

class Public::ItemsController < Public::ApplicationController
  def index
    if params[:genre_id]
      @genre = Genre.find(params[:genre_id])
      @items_all = Item.joins(:genre).where(genre: @genre, is_active: true)
    elsif params[:search]
      @keyword = params[:search]
      @items_all = Item.search(@keyword)
    else
      @items_all = Item.joins(:genre).where(genre: { is_active: true }, is_active: true)
    end
    @items = @items_all.order(id: "DESC").page(params[:page]).per(8)
  end

  def show
    @item = Item.find(params[:id])
  end
end

〜解説〜

このコードは、アイテムの一覧表示と検索機能を同じアクション（indexアクション）で処理しています。それぞれの条件（ジャンル指定、キーワード検索、全商品表示）によって異なる商品を@items_allに代入しています。

検索後に別のビューを表示する必要がない理由は、どの条件でも結果を@items_allに保存し、それを表示するビューが同じ（indexビュー）だからです。

ユーザーがキーワードで検索を行った場合、検索に一致する商品が@itemsに代入され、その結果が一覧表示されます。このとき、検索結果を表示する専用のビューを作成する必要はありません。

最後に、全てのシナリオで取得した商品は、IDの降順でソートされ、1ページに8つの商品を表示するようにページネーションされます。

したがって、検索後に別のビューを表示する必要はありません。
どのシナリオでも同じビュー（indexビュー）を使用して、@items_allに保存された商品を表示します。🙆🏻‍♀️

リーダブルコード要約

リーダブルコードで優れたコードとは
「他の人が読んだ時に、コードの意味を理解するまでの時間を短くできるコード」
と定義されている。大事や。

本を買わないといけないんだと思い込んでいましたが、普通に要約があるんですね！

もっと早く読むべきでしたが
チーム開発でリーダブルコードの重要性を身をもって感じたので
しっかり目を通してからポートフォリオに挑みたいと思います。

何周もして味を出しつつ理解をしていくような書籍だと説明されていたので、
あとで書籍もちゃんと買って詰まった時に見返せるようにしておきたいですね。

最後に

経験者とはいえ、コードレビューまで出来る＆それを共有してくださる方が同じ受講生にいることにとても刺激を受けました！

いろんな考えや意識を持ったエンジニアの方に出会い、
良い点をどんどん吸収していって自分の視野を広げていかないといけないなと感じています🧐

どんなに追い込まれても一つ一つ、
なぜこうなるのか？調べて突き詰める姿勢を忘れずにいきたいですね🥹