code review by sh-suzuki0301

[sh-suzuki0301]

@MinoriChikano アプリケーション全体を通して、改善点を記載します。 RubyやRuby On Railsの作法の観点ではなく、プログラミングの一般的な指摘とMVC構造のフレームワーク(以下、FW)という広い観点での指摘します。

[WIP]は指摘コメント作成途中のコメントです。

[sh-suzuki0301]

/.DS_store https://github.com/MinoriChikano/Section/blob/master/.DS_Store

上記のファイルは不要なので.gitignoreに追加をしましょう。 Macで自動生成されるフォルダやファイルのmeta情報を格納しているファイルです。

[sh-suzuki0301]

[WIP] Controllerの役割

MVC設計でのFWにおいてのControllerの役割はデータ入力に対して、Modelへのデータ受け渡し、Viewへ画面表示の指示を促すといった仲介役のようなものとなります。

なので具体的なデータ取得クエリの記述はModelへメソッドとして移植しましょう。

Modelのインスタンスを生成してる場合は、Modelは処理を移植できると考えても良いです。

こうすることで、例えば複数箇所でよく使うデータ取得の処理を、Modelから該当メソッドを各処理で呼び出せるので、クエリの重複記述を避けることができます。

全ての該当箇所を記載しませんが、上記観点で一度見直してみてください。

[sh-suzuki0301]

重複処理を関数化する

全く同じ処理が２度以上登場する場合は関数として共通化できます。

該当のController内でのみ使用する場合であればprivate関数として重複処理を抽出しましょう。 他のControllerや他処理でも使用する場合は、ユーティリティークラスやヘルパー(railsでの名称ではない)に関数を作成してアプリケーション全体の共通処理として呼び出せるようにしましょう。

https://github.com/MinoriChikano/Section/blob/master/app/controllers/audios_controller.rb#L19-L23

https://github.com/MinoriChikano/Section/blob/master/app/controllers/audios_controller.rb#L41-L43

applemusic = params[:audio][:reference]
applemusic.slice!(0,23)
@audio.reference = applemusic

そうすることで、共通化した関数を呼び出すのみで、上記のコードが下記のようにまとまります

どこかに共通関数として定義
class Hoge
def format_audio_reference(reference)
  reference.slice!(0, 23)
end

必要な箇所で関数の呼び出し
@audio.reference  = Hoge.format_audio_reference(params[:audio][:reference])

[sh-suzuki0301]

重複しているメッセージの文字列リテラル

各所で同一の記述があるメッセージを見かけます。 こういった共通の値は、アプリケーション共通の定数として定義しましょう。

理由としては、仮に 権限がありません を 権限がありません！！！ に修正しなくてはいけない時に、アプリケーション全体で修正箇所が10,000箇所あった場合どうでしょうか？ 実際はエディタの置換ですぐに修正はできますが、抜け漏れが発生する場合もあり、 テストでの確認も大変な工数となってしまうためです。

定数化をして定数として呼び出している場合は、定義をしている箇所を修正するだけで、 定数を使用している箇所全てに修正が反映されるので保守性が高いと言えます。

flash[:notice] = "権限がありません"

https://github.com/MinoriChikano/Section/blob/master/app/controllers/audios_controller.rb#L79 https://github.com/MinoriChikano/Section/blob/master/app/controllers/audios_controller.rb#L87 https://github.com/MinoriChikano/Section/blob/master/app/controllers/audios_controller.rb#L94

Railsでのベストプラクティスは不明ですが、下記記事のようなことを意図しております。 https://zm.hateblo.jp/entry/2020/10/14/212824

基本的にアプリケーション固有の数値や文字列を直接記載するのは避けるようにするのがベターです。 (ハードコードやマジックナンバーといいます)。

[sh-suzuki0301]

[WIP] 例外処理

[MinoriChikano]

@sh-suzuki0301 この度はレビューありがとうございます。

/.DS_store

上記はgitignoreに記載致しました。

重複処理を関数化する

こちらaudio controller内のprivateメソッドとして定義致しました。

applemusic = params[:audio][:reference]
applemusic.slice!(0,23)
@audio.reference = applemusic

各アクション内で下記メソッドを呼び出しております。

 def cut_url_reference_form
 applemusic = params[:audio][:reference]
 applemusic.slice!(0,23)
 @audio.reference = applemusic
 end

宜しくお願い致します。