Challenge 3 / 横田 俊博 #283
Challenge 3 / 横田 俊博 #283
Conversation
|
Sider detected 1 error and 1 warning on analyzing the commit b29c350. We recommend fixing them as possible by updating the dependencies, configuring the analysis tool, configuring If you have problems or questions still, feel free to ask us via chat. 💬 You can turn off this notification if you don't want to receive it from now on. |
|
This is a very good PR! |
| @@ -0,0 +1,7 @@ | |||
| class CitiesController < ApplicationController | |||
| def index | |||
| cities = City.eager_load(houses: :energies) | |||
There was a problem hiding this comment.
これ、houses を経由せずとも
| cities = City.eager_load(houses: :energies) | |
| cities = City.eager_load(:energies) |
と書けるのではないか。
|
|
||
| private | ||
|
|
||
| def energy_attributes |
There was a problem hiding this comment.
このメソッド名は、 as_json とかにするのが好みであるし、
Rails の convention に従っている気がする。
| validates :has_child, presence: true | ||
|
|
||
| def save | ||
| ActiveRecord::Base.transaction do |
| { | ||
| name: city.name, | ||
| data: city.energies | ||
| .select(:energies_production) |
There was a problem hiding this comment.
このロジック全体的に City モデルのメソッドにする方が良かったかもしれない。
（が、そうすると逆に Service に切り出す意味がなくなるか・・・）
There was a problem hiding this comment.
8c308c8 で対応しました。
このロジック全体的に City モデルのメソッドにする方が良かったかもしれない。
（が、そうすると逆に Service に切り出す意味がなくなるか・・・）
チャート出力のための処理を Service に切り出すかどうかですが、
ロジックがそこまで大きくないこともあり切り出す必要は無かったかもしれません。
（が、この技術課題でService層を作りたかったという理由で結果として切り出しました。）
| end | ||
|
|
||
| def call(cities) | ||
| cities.select(:id, :name).map do |city| |
There was a problem hiding this comment.
map を使わずに 1回のデータベースへの問合せにできる方法があります。
都市が今回は３つなので、このコードでも問題ありませんが。
There was a problem hiding this comment.
最終的に
name に 都市名,
data に 年月 を key, エネルギー生産量 を value
とするハッシュの配列
を出力する必要があり、いい感じのクエリは作れませんでした。
なんとなく GROUP BY に city も含める方向性かなと思いましたが。。
SELECT
c.name,
e.year,
e.month,
SUM(e.energy_production)
FROM
cities c
RIGHT OUTER JOIN
houses h ON c.id = h.city_id
RIGHT OUTER JOIN
energies e ON h.id = e.house_id
GROUP BY
c.name, e.year, e.month;
|
|
||
| def house_form_params(row, city) | ||
| { | ||
| firstname: row['Firstname'], |
There was a problem hiding this comment.
id というカラムは連番にして、 house_id カラムを hosues テーブルに追加した方がよかったでしょうか？
|
@cuzic レビューいただきありがとうございました。ご指摘いただいた部分を対応、コメントしました。 |
|
This is a very good PR |
2 participants
概要
Challenge 3 - Web Application
Deployed URL
https://enechange-ch3.herokuapp.com
不十分な点
productionのDBがdevelopment,testと合っていない。development,testでmysqlを使っているので、productionでもmysqlを使うようにする。開発環境が Docker 化されていない。
市ごとのエネルギー生産量月推移ページでグラフ、テーブルが見切れてしまう。
datepicker、 テーブルの方はページネーションのライブラリを活用するなどして、UXを改善する。テストのカバレッジが不十分。
model specのunit testを追加する。viewに若干ロジックが存在してしまっている。active_decoratorやdraperを活用して、プレゼンター層を導入する。特にアピールしたい点、独自の工夫点
パーフェクトRails 第11章 エキスパートRailsで紹介されているようなドメイン駆動設計を意識したクラス設計を行いました。