Rails: 昔プログラミングの勉強の一環で作成したウェブアプリを振り返ったりツッコミを入れてみる

こんにちは。今年もアドベントカレンダーの時期がやってきました。
いきなりですが、こちらを御覧ください。
https://triplaning.herokuapp.com/

これは昔、私がプログラミングを勉強し始めの頃に、勉強の一環としてなにかウェブアプリを作ってみたいと思い、Ruby on Railsを使って作成したウェブアプリです。

今回、この作成したアプリに対して、振り返り&いろいろツッコミをいれていこうと思います。

はじめに

まずはじめにこのアプリがどんなアプリなのかを一応説明すると、旅行のプラン(行程表)を作成するアプリです。
(私が旅行が好きなので、旅行に関するアプリだと作るモチベーションがより湧くかなと思い、このテーマにしたような記憶があります。)

サイトのトップページのGIF画像にあるように、行きたい場所を入力していき、各場所を回る順序を入れると、Google Maps API の中のAPI(だった記憶)を使って2地点の距離や移動にかかる時間やなどを算出した結果を表示します。その後、すべての入力が終わると、作成した工程表をPDFとして出力したり、メンバーにメールで共有する機能などがあります。

※ 実際に使用される想定でこのアプリを作ってはいないので、もし旅行の行程表を作成したいという人がいれば、他にもサービスはたくさんありますのでそちらをご利用ください。

また、あくまでプログラミングを始めたばかりの私が、勉強目的で使って作ってみたというものなので、
そのあたりを踏まえて上で挙げたウェブアプリも本記事もネタとして見ていただければと思います。

では早速、みていきます。

まずは見た目

まず見た目の部分ですが、トップ画面を見た感じだと、横並びの項目の高さのずれや、中央にあるべきものがずれていたりなどいろいろありそうです。

おそらく見た目でいうとトップ画面は一番こだわって作った思うので、当時の精一杯でCSSを書いてはいたことでしょう。(気合は伝わってきました...)

粗を探すとキリがなさそうなので見た目はこれくらいにします。

操作もしてみる

いくつか操作もしてみようと思います。

まずはフェイスブックで新規登録/ログインを試してみましたが、これはエラーとなり登録及びログインができませんでした。おそらくですが、FacebookのAPIは頻繁にアップデートが行われている印象があるので、そこでおいていかれてしまった感じでしょうか。

また、ログインすると、プランに対して場所を紐付け登録できるはずなのですが、こちらの登録もできなくなってました。おそらく、何かしらAPIを使って地名から緯度経度を取得するようにした記憶があるので、↑と同じような理由ではないかと想像しています。

せっかくなのでコードもちらっと見てみる

ということで、まずはモデルのコードを見てみました。

ぱっと目に飛び込んできたのがこのコードです。
ActiveRecord::Baseを継承したモデルの中のインスタンスメソッドです。

# 行きたい度に応じて、★を表示させる。
# とりあえず、場所にひもづくピンのなかで、一番目の情報を表示する。
def show_star
  if self.pins[0].present?
    if self.pins[0].want == 0
      "★"
    elsif self.pins[0].want == 1
      "★★"
    else
      "★★★"
    end
  end
end

優先度に応じてそれを★の数で表現したかったのだろうなーと思います。

• selfは不要なのでは？
• 対象がすべてself.pins[0].wantなのでcase、whenを使ったほうが見やすくなりそう？
• メソッド名のshow_starという命名が斬新で面白い(わかりにくい)
• そもそも表示に関するメソッドなので、モデルに書かずに、Decoratorなどを導入してそちらに書いたほうが良さそう？

たった数行のコードですが、他にもいろいろ突っ込みどころはありそうです。

思いのほかモデルにコード自体が少なかったので、コントローラも見てみました。

まずはコントローラ内のpublicなメソッドです。場所の情報を削除する処理かと思います。

def destroy
  plan_id = Place.find(params[:id]).plan.id
  @place.destroy
  @reorder = Place.where(plan_id: plan_id)
  @reorder.each_with_index {|route, i| Place.update(route, {:route => i + 1})}
  respond_to do |format|
    format.html { redirect_to plan_path(plan_id), notice: '削除しました' }
    format.json { head :no_content }
  end
end

削除した後に、route(並び替えの順番)を1つずつ更新しているようですが、
これらの処理はセットなのでトランザクションで囲ってあげたほうが良さそうに見えました。

続いてもpublicなメソッドです。

def pdf
  respond_to do |format|
    format.html { redirect_to :action => 'pdf', :format => 'pdf', debug: 1 }
    format.pdf do
      @plan = Plan.find(params[:id])
      @places = @plan.places.order(:route)
      render pdf: 'pdf',
        layout: 'pdf_layout.html.erb',
        encoding: 'UTF-8',
        no_background: false
    end
  end
end

pdfというアクションで工程表をPDFとして出力する処理だと思いますが、
要求されたフォーマットがpdfであればpdf、htmlであればhtmlで出力するのがいいと思うので、pdfというアクションはやめたほうが良さそうと思いました。あとよく見てみるとhtmlの要求をpdfの要求としてリダイレクトさせてますね...

続いてはメール送信の処理です。(あるコントローラ内のpublicのメソッドからの抜粋)

# deliverメソッドを使って、メールを送信する。メンバー全員に送信する
members.each do |member|
  MemberMailer.plan_finish_email(@plan, member).deliver
end

一人ひとりそれぞれにメール送信するならば、直ちに送信するのではなく、非同期にした方が良いかもと思いました。

最後にビューも見てみようと思いファイルを開いてみたところ、インデントずれなど多く見る気が削がれたのでそっとファイルを閉じました...

まとめ

• プログラミングを始めたばかりの頃に作ったアプリを見たり、書いたコードを見てみて、すごく懐かしい気分になりました。
• 予想通りではありましたがツッコミどころの多いコードでした。
• レベルはともわれ昔の自分にツッコミを入れられたので、少しでも自分の成長を実感できたのはよかったです。
• 気分が乗れば年末年始の時間がある時にいろいろリファクタリングしてみたいと思いました。

おたより発掘

なにこれ僕もやりたいけど昔のコードなんてどこにあるんや…… /Rails: 昔プログラミングの勉強の一環で作成したウェブアプリを振り返ったりツッコミを入れてみる https://t.co/k4UscNIUoM

— すろっくさん (@srockstyle) December 25, 2018