# ポートフォリオ(散歩くじ)レビュー

## サービスURL
https://www.walking-lot.com/

## 概要
- 「散歩くじ」は散歩の行先と目的をくじ引きのように提案するサービスです。
- 行先のタイプと出発地点を登録することで、くじの結果（行先とアクティビティ）が表示されます。
- 行先は、出発地点から半径約2km以内のスポットをランダムに取得して提示します。
- アクティビティは、予め用意したものの中から行先のタイプに応じたものをランダムで選択し、提示します。
- また、「寄り道スポット」として、目的地の他にも同様のスポットをユーザーに提示します。
- くじの結果はSNSで共有することができます。

# レビュー

## Kei

大前提として、操作方法もシンプルで手軽に遊べる面白いアプリだと思いました！！
全体的なやわらかいデザイン的もアプリの雰囲気とマッチしていると思います！くじを引くボタン押すと、何が出るんだろう？とワクワクする感覚がありました！
レビューをいくつか書かせていただきましたが多々間違っている箇所もあるかもしれません🙇‍

### UI
- スマホで右上メニュークリックで表示するモーダルがメインビジュアル内のテキストやボタンよりも下層に配置されている？
- くじの結果が好みではないのでもう一度引き直したい、という需要もあるかもしれないので、出発地点の変更はせずにもう一度くじを引く機能が有っても良いかもしれません！
- 出発地点の検索サジェストですが、住所が最後まで見えるようになると嬉しいです！デザインの都合上難しいかもしれませんが…
- くじ引きに失敗したとき（「行きたい場所のタイプ」を選択せずにくじを引いてしまった場合や、くじを引いた結果スポットが見つからなかった場合）に出発地点の情報が消えてしまい、改めて入れ直す手間があるので、出発地点情報は保持できると良いと思いました！（よくあるフォームのvalidationに引っかかったとしても値が保持されているイメージ）
- 404ページを実装しても良いのかなと思いました！

### コード
- routes.rbの上部にあるルーティング一覧表示ですが、調べたところannotationの機能ですかね？こんなことできるなんて初めて知りました！
- 管理者権限はアクティビティ管理だけですかね？ユーザー一覧などは見られない？rails_adminを使って実装されていたらすいません（使ったことがないもので）
- [Notifications::ReadsController#create](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/notifications/reads_controller.rb#L4-L9)の中で`update!`メソッドが使われている？だとするとcreateアクションではなくてupdateアクションで良い気がします！
- ↑のコントローラーの使い方、[この記事](https://postd.cc/how-dhh-organizes-his-rails-controllers/)や[この記事](https://tech.kitchhike.com/entry/2017/03/07/190739)を思い出しました！
- ただの入力間違いだとは思いますが[エンドポイント設計](https://github.com/satoshitodaka/my_portfolio/blob/master/%E6%95%A3%E6%AD%A9%E3%81%8F%E3%81%98/summary.md#%E3%82%A8%E3%83%B3%E3%83%89%E3%83%9D%E3%82%A4%E3%83%B3%E3%83%88%E3%81%A8%E3%82%B3%E3%83%B3%E3%83%88%E3%83%AD%E3%83%BC%E3%83%A9%E3%83%BC)では「通知を既読にする」機能のHTTPリクエストがGETになっていました（エンドポイント設計と実際の実装では若干変更ありましたかね？エンドポイント設計ではnamespaceがmypageになっているので）
- Lotモデルのset_destinatioメソッド内の[最後のif文](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/models/lot.rb#L58-L62)は`if...elsif`ではなく`if...else`で書ける？もしそうであれば三項演算子を使って短くできそう

self.photo_url = destination_informations['photos'] ? destination_informations['photos'][0]['photo_reference'] : 'no_image'

- [ここ](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/lots_controller.rb#L8-L12)も以下のように三項演算子使える？

# app/controllers/lots_controller.rb
@lot = current_user ? current_user.lots.new(lot_params) : Lot.new(lot_params)

- 好みの問題かもしれませんが、関連先のテーブルを作成するときは`new`ではなく`build`メソッドを使うイメージなのですがどうでしょう？（エイリアスなのでどちらでも良いという結論になりそう笑）→例えば[ここ](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/lots_controller.rb#L9)
- （自信無いのでだいそんさんに解説してもらいたいです🙇‍）[LotsController#create](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/lots_controller.rb#L14-L32) の中でif文がネストしているため、もっと綺麗に書けないかなと思うのですが、以下のような書き方はできますか？（すいません、renderの返り値に関して自信無いです）

render :new, status: :unprocessable_entity and return unless @lot.valid? # renderの引数が2つあるのでこのままではandが機能しないかもしれません

if @lot.nearby_locations['status'] == 'OK'
    # 省略
elsif @lot.nearby_locations['status'] = 'ZERO_RESULTS'
    # 省略
end

- ↑の`if...elsif`文ですが`if..else`文で書いていない理由がありますかね？`nearby_locations['status']`には`'OK'`と`'ZERO_RESULTS'`以外が入ることは無いのでしょうか？
- Admin::ActivitiesController内の[create_notification_about_activity_updatedメソッド](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/admin/activities_controller.rb#L37-L43) ですが、少しメソッド名が長い気がするので`create_notification`だけでも良いと思います！あと、呼び出しもとで引数に@activityを指定されていますが、インスタンス変数なのでメソッド内から直接参照できると思うので引数は不要かと思います（規定として、インスタンス変数はメソッド内から直接参照しない方が良いというものがあったらすいません）

- [Mypage::ActivitiesController#destroy](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/mypage/activities_controller.rb#L45-L46)が空？（ルーティングは定義されているっぽい）

- [Mypage::ActivitiesControllerの2つのプライベートメソッド](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/mypage/activities_controller.rb#L53-L67) が似てるのでひとつにできないですかね？呼び出しもとの引数が長くなって微妙かもですが…（[create_notification_about_activity_updatedメソッド](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/admin/activities_controller.rb#L37-L43) も似ているので、`app/controllers/concernes`配下にmoduleを作ってincludeしても良いかもしれません）

# 以下のメソッドを定義し、引数を指定して呼び出す

def create_notification(title, url)
      notification = Notification.create!(
        title: title,
        url: url
      )
      notification.notify(User.admin)
end

- 配列化したものを[ここ](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/models/notification.rb#L19)で平坦化された理由を教えていただきたいです！（[ここ](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/admin/activities_controller.rb#L42)で指定した引数を配列化したいということですかね？）

## sari
ユーザー登録なしでも大部分のサービスを楽しめたり、使い方やヒントが随所に設置されていたりと、ユーザー目線の心配りを感じました。
アクティビティもわくわくします。
一度実際に使って散歩したいと思います！

### UI
- フッターメニューのアイコンに文字が添えられていると初見でも迷いづらいかなと思います。
（私が以前dysonさんから指摘された内容です…🙋）

- 使い方ページのページ内リンクの飛び先が、少しずれているようです。ヘッダーメニュー分の高さを含めたらピッタリになるかと思います

- 使い方ページの番号部分が、フッターメニューより上層に表示されているようです。
<img src="https://i.imgur.com/PvymxZ2.png" width="300px">

- 散歩くじで提案される散歩コースの往復の所要時間の目安がどこかに書かれていると、散歩くじを使えるタイミングが分かってありがたいです。もしくは、散歩の所要時間をユーザーが選択できて、それに応じてGoogle Map APIへのリクエストのパラメータを変えるとか？

- 寄り道スポットが０個の場合の表示は、寄り道スポットの欄まるごと非表示に？
<img src="https://i.imgur.com/bPIieE7.png" width="300px">

### コード
- `OtherPlace` = 「寄り道スポット」というより「目的地の第二、第三候補」に表現として近いのかなと感じました！
「寄り道」というと、個人的には出発地点と目的地点の中点を中心とした円内にある点、のようなイメージです。目的地と逆の方向でそこそこ歩くような地点が寄り道として提案されるとギャップを感じそうです。
- 散歩くじの結果の出し方は、
[出発地から半径2km以内で目的地として条件に合う検索結果](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/models/lot.rb#L41)のうち[３件までランダムに取得して配列に入れ](https://github.com/satoshitodaka/walking_lot/blob/46ba77e078481882f7df269b0d9ff72d7d12fc34/app/controllers/lots_controller.rb#L52)、１個目を「目的地」、2~3個目を「寄り道スポット」に設定している、という理解です。
- 「寄り道スポット」の提案される範囲と寄り道の個人的なイメージ。
例）出発点：渋谷駅、目的地：目黒川、寄り道スポット：明治神宮
<img src="https://i.imgur.com/jLjjoVD.jpg" width="600px">

## だいそん
### サービス面
- 使い方ページがあって親切で良い
- 使い方がシンプルで良い
- ハードルが高いですね笑
- [](https://gyazo.com/c42e39bf42262412504c2d58690c79c8)
- 寄り道、良いですね
- [](https://gyazo.com/0331bc88ba48861f1d7fdcdb1ca4d557)
- クリックして詳細に飛べないんでしょうか？
- UIはなんだかんだテンプレートを使うのが良い気がする。

### 実装面
- ルーティングの定義が書かれてて読みやすい
- https://github.dev/satoshitodaka/walking_lot/blob/5255ab3dacae35d1d97a55da5cfb9b577758a80a/config/routes.rb#L118
- が、rails標準のルーティングは省いてもいいかも。active storageとかation mailboxとか。
- コントローラではなくモデルか何かに書いた方が良いかも
- https://github.dev/satoshitodaka/walking_lot/blob/5255ab3dacae35d1d97a55da5cfb9b577758a80a/app/controllers/lots_controller.rb#L52-L62
- 2000は定数で持っておいた方が良いかも。そして距離を引数で受け取るようにするもあり。
- https://github.dev/satoshitodaka/walking_lot/blob/5255ab3dacae35d1d97a55da5cfb9b577758a80a/app/models/lot.rb#L41-L48

DEFAULT_RADIUS = 2000

def get_nearby_locations(radius: DEFAULT_RADIUS)

みたいなイメージ。（動くかどうかは未検証です）

- このバリーでションって必要でしたっけ？
- https://github.dev/satoshitodaka/walking_lot/blob/5255ab3dacae35d1d97a55da5cfb9b577758a80a/app/models/lot.rb#L37-L38
- `self.photo_url = 'no_image'`はバグの温床になりそうな気がした
- https://github.dev/satoshitodaka/walking_lot/blob/5255ab3dacae35d1d97a55da5cfb9b577758a80a/app/models/lot.rb#L61-L62

- createアクションが肥大化してる
- https://github.com/satoshitodaka/walking_lot/blob/main/app/controllers/lots_controller.rb#L6-L34

  def create
    ActiveRecord::Base.transaction do
      if current_user
        @lot = current_user.lots.new(lot_params)
      else
        @lot = Lot.new(lot_params)
      end

      if @lot.valid?
        @lot.get_nearby_locations

        if @lot.nearby_locations['status'] == 'OK'
          @place_order_numbers = set_place_order_number(@lot)
          @lot.set_destination(@place_order_numbers)
          @lot.save

          LotActivity.create_with_lot(@lot) # 従属するモデルLotActivityを生成
          OtherPlace.create_with_lot(@lot, @place_order_numbers) # 従属するモデルOtherPlaceを生成

          redirect_to lot_path(@lot), success: 'くじを作成しました'
        elsif @lot.nearby_locations['status'] = 'ZERO_RESULTS'
          flash.now[:info] = '近くにスポットがありませんでした。条件を変えてもう一度引いてください。'
          render :new, status: :unprocessable_entity
        end
      else
        render :new, status: :unprocessable_entity
      end
    end
  end

こんな感じで書けるようになったらベターかなと思いました。

def create
  @lot_form = LotForm.new(lot_params: lot_params, current_user: current_user)
  if @lot_form.save
    redirect_to lot_path(@lot_form.lot), success: 'くじを作成しました'
  else
    flash.now[:info] = @lot_form.errors.full_messages
    render :new, status: :unprocessable_entity
  end
end

class LotForm
  include ActiveModel::Model
  attributes :current_user
  attributes :lot
  
  def initialize(lot_params:, current_user:)
      self.lot = Lot.new(lot_params)
      self.current_user = current_user
  end
  
  def save
    # ここでゴリゴリロジック描く
    
    # 失敗した時
    errors.add(:base, "なんかエラーメッセージ")
  end
end

## junnosuke
使っていて楽しかったです。UIだけレビューさせていただきます。
コードまで追えませんでした。すみません。
### UI
- 使い方のフローでページに遷移するところを少し下に調整したほうが良さそうです。（上部が隠れてしまってる？）
- さんぽクジのところのUIで真っ白だと同化して見辛いかなと感じました。→枠線が太くするor背景色をつける
- （できたら）くじで選ばれたところまでのルートと時間とか出ると助かる→マップにアクセスすると見れるが、その前で見れたらいいかなと思います。
- アクティビティーに見出しがあるといいかなと。
[](https://gyazo.com/b97aa0271222aff02aaf5cf866f105ea)
- 出発地を選択して、いきたい場所のタイプを入れ忘れた時に、出発地が消えちゃって再入力が必要になるので、エラーになった時、データが残っていると助かります。

## かろりーな
先日出先で使ってみました！楽しかったです。
- テキストで出発地点を入力する→地図上のピンを押して設定→くじを引く　になると思うのですが、テキストで出発地点を入力するとそのまま出発地も設定できたら嬉しいなと思いました。
[](https://gyazo.com/dbd11678bf783bcd76324a5c2c369c87)

## みけた
みなさん書かれているとおり、気軽に試せて、楽しいアプリだなと思いました！　早速試そうと思います！
以下のページについてユーザー視点で気になったところを記載します！(https://www.walking-lot.com/lots/new)

- 「左の所在地を入力する → 出てくる複数のピンから選んでクリック　→ くじを引く」というステップを踏む中で、「出てくる複数のピンから選んでクリック」が必要なことに気づきづらい。以下のような対策をするといいかも。
- 登録のヒントをクリックすると、gifの動画出てくる
- くじを引くが「出てくる複数のピンから選んでクリック」まで活性化されない
- どこまで情報がサーバー側に送信されるのか、保存されるのか気になるので、そのあたりを明記してくれるとありがたい
- 範囲指定をユーザー側で指定できるとありがたい
- 「アクティビティにチャレンジする」の部分が、若干唐突に出てくるような気がして、初見だと何を指しているのか意味が分かりづらかったです

あと、散歩のススメのページが好きです。Wantedlyだと給与や条件面をあえて求人ページに書けない仕様になっているようですが、2kmというところでFIXになっている背景には、もしかしたらそうした理由があるのかなと想像しました。
https://www.walking-lot.com/tips_to_enjoy

## maru
面白いアプリですね！
ちょっと触ってみた感想を一般ユーザー目線でレビューさせていただきます。
- 出発地点の登録部分で、「マップ上で指定してください」という説明があると親切かなと思いました。出発地が「自動入力されます」とありますが、感覚的に出発地の部分に直接入力したくなりました。どうしたら自動入力されるのかがわかりづらいかも？
- 地図上で「ここから歩く」を押したときに、ボタンの見た目に変化がないので出発地に反映されているかどうかがわかりづらいと思いました。何回も押してしまいました。
- 散歩くじの引き直しをすると出発地点が消えるのは私も気になりました。
- 寄り道スポットのみの引き直しもできたら、より良いかなと思います。目的地が気に入っても寄り道スポットが気に入らない場合が考えられるので。
- 以前おっしゃっていた散歩アクティビティが記載されている部分にタイトルがあると良いなと思います。アクティビティの部分がどういう意味なのか初見ユーザーは分かりづらいと思います。
- 散歩のメリットを紹介しているのいいですね！
- 頑張ってください！