はじめに
軽く自己紹介すると、エンジニア歴9ヶ月のひよっこです。
先日、ふと自分が未経験の時に作ったアプリのコードを見返すと、ツッコミどころが盛り沢山だったので、設計、実装を見直し改修しました。
エンジニアになってまだ僅かですが、過去に自分が書いたコードにメスを入れたことで今になってわかる気づきが得られたので記事にしようと思いました。
どんなアプリ?
Youtube上にある #4kwalk, #walkingtour などのバーチャルツアーの動画を活用し、まるで世界中の街中にいるかのような雰囲気を体感できるアプリです。→気になった方はこちら
バックエンドをRails API、フロントエンドをVue.jsで作っています。また、地図やYoutubeの動画をアプリから参照できるように、Geocoding API や YouTube Data API などを導入しています。
今回の改修では設計と実装に手を加えてつつも、仕様は変えないようにしました。
詳細
上のGIF動画でもあるように、作成したアプリには地図上のスポットをクリックしたと同時にYoutubeで取得した動画を一覧を表示する機能があります。手を加えたのは、この機能に関わる設計、実装の部分です。
設計について
問題部分
設計、詳細にいうとDB設計ですが元は以下のようになってました。
赤が地図上のピンを表す地点テーブル、青がYoutube動画情報を格納する動画テーブルとなります。
何かおかしいことに気づきませんか.....?
そうです...地点テーブルと動画テーブルが紐づいていないのです!
本来であれば、地点テーブルと動画テーブルの関係性は1対多になるはずです。しかし、紐付けすらされておらず動画テーブルは独立したテーブルとなっていました。
なぜ、このような設計になっていたのか?
開発者本人ですが、すっぽり記憶が抜けて落ちてしまいはっきりとした理由はわかりませんでした;
ただこのような設計により、API側で動画を取得できるようにするためには、地点名をクエリパラメータで指定しなければならず、以下のような不格好なURLになってしまってました。
- クエリパラメータを含めたURL
api/v1/videos?spot_name='ロサンゼルス'
従って、動画の取得も以下のような処理になっています。
- 動画の取得
Video.where(spot: params[:spot_name])
改修内容
地点テーブルと動画テーブルを1対多になるよう紐付けます。
加えて、動画テーブルからカラムarea(国名)
、spot(地点名)
を削除します。
また、動画一覧を取得するAPIのURLもルーティングの設定により変更します。
- 変更後のURL
/api/v1/spots/{spot_id}/videos
これにより、クエリパラメータに値を含めなくともAPI側でパスパラメータのspot_id
から動画を取得できるような実装に変更できました!
- 動画の取得
Video.where(spot_id: params[:spot_id])
設計の気づき
テーブル設計で最適な関連付けが行われていないことで、ルーティングの定義やデータの取得処理に悪影響を及ぼし、分かりづらい実装になってしまうことがある。
実装について
問題部分
一覧で表示する動画を取得する処理に関連する部分は次のとおりです。
class Api::V1::VideosController < ApiController
include YoutubeApi
# 一覧で表示する動画を取得
def index
# スポットに関する動画を取得
@search_results = Video.where(spot: params[:spot_name])
@response = []
if saved_and_recent?
@search_results.each do |result|
@response << { video_id: result.video_id, title: result.title, thumbnail: result.thumbnail, view_count: result.view_count, published_at: result.published_at.strftime("%Y/%m/%d") }
end
elsif saved_and_old?
begin
# Youtube APIから動画一覧を取得
get_videos(params, params[:iso])
# 取得した動画一覧をDBに更新保存
update_videos(@api_responses, @search_results)
# 取得した動画一覧をレスポンスで返す
set_videos
rescue
@response = { error: ResourceNotFound }
end
else
begin
# Youtube APIから動画一覧を取得
get_videos(params, params[:iso])
# 取得した動画をDBに保存
save_videos(@api_responses, params)
# 取得した動画一覧をレスポンスで返す
set_videos
rescue
@response = { error: ResourceNotFound }
end
end
render json: @response.sort{|x, y| x[:view_count] <=> y[:view_count] }.reverse
end
private
# 地点に関する動画を全て取得
def set_videos
@search_results = Video.where(spot: params[:spot_name])
# 取得した動画一覧をレスポンスで返す
@search_results.each do |result|
@response << { video_id: result.video_id, title: result.title, thumbnail: result.thumbnail, view_count: result.view_count, published_at: result.published_at.strftime("%Y/%m/%d") }
end
end
# 地点に関する動画をDBから取得できる & 動画が最近更新されている
def saved_and_recent?
@search_results.present? && @search_results.first.recently?
end
# 地点に関する動画をDBから取得できる & 動画が最近更新されていない
def saved_and_old?
@search_results.present? && !@search_results.first.recently?
end
end
module YoutubeApi
require 'google/apis/youtube_v3'
# Youtube
def get_videos(query, region, after: 2.years.ago, before: Time.now)
youtube = Google::Apis::YoutubeV3::YouTubeService.new
youtube.key = Rails.application.credentials.google[:youtube_api_key]
@api_responses = []
# 検索結果を取得
search_results = youtube.list_searches(
:snippet,
type: "video",
q: query[:spot_name_ens] + " " +"walking tour 4k",
max_results: 24,
region_code: region,
# HD 動画のみ
video_definition: "high",
# 20 分を超える動画のみ
video_duration: "long",
# 埋め込み可能な動画のみを検索
video_embeddable: true,
# 関連度が高い順
order: "relevance",
# 2年前より後のみ
published_after: after.iso8601,
# 本日より前のみ
published_before: before.iso8601,
fields: 'items(id(videoId), snippet(title, publishedAt,thumbnails(medium(url))))'
)
# 各動画の再生回数を取得(list_searchesメソッドでは再生回数を取得できないため)
search_results.items.each_with_index do |item, index|
video_id = search_results.items[index].id.video_id
video_results = youtube.list_videos(
:statistics,
id: video_id,
max_results: 24,
fields: 'items(statistics(view_count))'
)
view_count = video_results.items[0].statistics.view_count
snippet = item.snippet
thumbnail = snippet.thumbnails.medium.url
# { video_id・動画タイトル・サムネ・再生回数 } を返す
@api_responses << { video_id: video_id, title: snippet.title, thumbnail: thumbnail, view_count: view_count, published_at: snippet.published_at }
@api_responses
end
end
# Youtube動画を保存
def save_videos(results, query)
results.each do |result|
Video.create!(
area: query[:area_name],
spot: query[:spot_name],
video_id: result[:video_id],
title: result[:title],
thumbnail: result[:thumbnail],
view_count: result[:view_count],
published_at: result[:published_at]
)
end
end
# DBにあるYoutube動画を更新保存
def update_videos(latest_results, past_results)
past_results.each_with_index do |result, index|
result.update!(
video_id: latest_results[index][:video_id],
title: latest_results[index][:title],
thumbnail: latest_results[index][:thumbnail],
view_count: latest_results[index][:view_count],
updated_at: Time.current
)
end
end
end
コード量が多くて何してるのかよくわからないと思いますが、一覧で表示する動画は以下のような条件で取得しています。
- 地点に関する動画がDBに保存されていない場合、Youtubeから動画情報を取得しDBに保存した上で表示
- 地点に関する動画がDBに保存されている & 直近、動画情報が更新されている場合、DBに保存された動画情報を取得し表示
- 地点に関する動画がDBに保存されている & 直近、動画情報が更新されていない場合、Youtubeから動画情報を再取得しDBを更新した上で表示
この実装の改善できそうな点について大きく三つ述べます。
改善できそうな点
-
冗長なコード
似たようなコードが書かれた箇所がいくつかあり、コードの可読性は下げてしまっている -
分かりづらいメソッド名
例えば、 get_videos でDBから取得するのか Youtube API を使って取得するのかコメントで補足しないと明確にわからないようになっている -
ロジックの分離が中途半端
データの取得処理や保存・更新処理などのロジックをモジュールに切り出しているのはいいが、結局のところ条件分岐の処理はコントローラー側に役割を持たせてしまっているので、ロジックの構成がコントローラーとモジュールにまたがってしまっており切り出した意味を成していない
以上の点を踏まえて修正してみました!
class Api::V1::VideosController < ApiController
include Api::Video
def index
unless videos.first&.recently_updated?
fetch_videos
end
render json: videos,
root: 'videos',
adapter: :json,
each_serializer: VideoSerializer,
meta: {
area: spot.country,
spot: spot
}
end
end
module Api::Video
extend ActiveSupport::Concern
require 'google/apis/youtube_v3'
def initialize
@youtube = Google::Apis::YoutubeV3::YouTubeService.new
@youtube.key = Rails.application.credentials.google[:youtube_api_key]
end
#
# DBへの保存・更新処理を行う
#
def fetch_videos
videos.present? ? update_videos : save_videos
end
#
# Youtube APIで動画を検索し取得
#
def fetch_youtube_videos(after: 2.years.ago, before: Time.now)
@fetch_youtube_videos ||= @youtube.list_searches(
:snippet,
type: "video",
q: spot.name_ens + " " +"walking tour 4k",
max_results: 24,
region_code: spot.iso,
# HD 動画のみ
video_definition: "high",
# 20 分を超える動画のみ
video_duration: "long",
# 埋め込み可能な動画のみを検索
video_embeddable: true,
# 関連度が高い順
order: "relevance",
# 2年前より後のみ
published_after: after.iso8601,
# 本日より前のみ
published_before: before.iso8601,
fields: 'items(id(videoId), snippet(title, publishedAt,thumbnails(medium(url))))'
)
end
#
# Youtube動画の再生回数を取得
# - list_searchesメソッドでは再生回数を取得できないためlist_videosを使って取得
#
def view_count(item)
@view_count ||= @youtube.list_videos(
:statistics,
id: item.id.video_id,
fields: 'items(statistics(view_count))'
).items[0].statistics.view_count
end
#
# Youtube動画情報を収集
#
def youtube_videos
@youtube_videos ||= fetch_youtube_videos.items.map do |item|
{
video_id: item.id.video_id,
title: item.snippet.title,
thumbnail: item.snippet.thumbnails.medium.url,
view_count: view_count(item),
published_at: item.snippet.published_at
}
end
end
#
# 動画情報を保存
#
def save_videos
youtube_videos.each do |result|
spot.videos.create!(
video_id: result[:video_id],
title: result[:title],
thumbnail: result[:thumbnail],
view_count: result[:view_count],
published_at: result[:published_at]
)
end
end
#
# 動画情報を更新
#
def update_videos
videos.each_with_index do |result, index|
result.update!(
video_id: youtube_videos[index][:video_id],
title: youtube_videos[index][:title],
thumbnail: youtube_videos[index][:thumbnail],
view_count: youtube_videos[index][:view_count],
updated_at: Time.current
)
end
end
def videos
@videos ||= spot.videos
end
def spot
@spot ||= Spot.find(params[:spot_id])
end
end
修正ポイント
- シリアライザの活用
active_model_serializers を導入したことでレスポンスの形式を簡易なコードですっきり整形 - キャッシュの活用
何度も呼ばれる関数は||=
を使うことで何度もDBアクセスが走らないようにして時短な処理に修正 - モジュールの変更
モデルのフォルダ内のにあったモジュールから、コントローラーのフォルダ内に移動 - コントローラーのスリム化
DBへの保存・更新処理を実行するかの条件分岐はコントローラーに持たせ、保存・更新処理はモジュールで全て完結するよう修正
修正結果
設計と実装の中身を中味を改修したことで、操作性はそのままに動画一覧表示の処理速度が1/2にまで短縮されました✨
おわりに
今回の出来事を通じて、昔の自分のコードをレビューして振り返ることは、今の自分がどれほど成長したのか実感できるいい機会になるなと感じました。また、仕様は変えず設計や実装処理の内容を修正することでパフォーマンスを向上させる作業は、実務でも大奥はないと思うので、自分の力を試すことができるなとも思いました。
眠ったままの個人サービスをお持ちの方は、ぜひ過去の自分のコードを振り返り、今の自分のレベルを体感してみてください。
ここまで読んで頂いた皆様、ありがとうございました!