repositoryのGetEditionPlayStatsを実装する #1403
Conversation
|
Migrate lint ✅ Lint output |
1 similar comment
|
Migrate lint ✅ Lint output |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1403 +/- ##
==========================================
+ Coverage 50.00% 50.51% +0.51%
==========================================
Files 135 136 +1
Lines 11810 11979 +169
==========================================
+ Hits 5905 6051 +146
- Misses 5601 5620 +19
- Partials 304 308 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
ありがとう、とてもよく書けていると思います。
SQLで書くのもたぶんできなくはないんですけど難しいので、今回は読みやすさと実装しやすさを優先して全件取得からGoで集計の今の方針でいいと思います。
(#1393 をみたのですが，これは一方の時間帯のログとして処理していそう？)
これがよくわからなかったんですが、説明をお願いしてもいいですか？
このPRのロジックは多分これで合っていると思います。
| )) | ||
| } | ||
|
|
||
| sort.Slice(hourlyStats, func(i, j int) bool { |
There was a problem hiding this comment.
sortパッケージはジェネリクスが入る前の機能なので、代わりにslices.SortFuncを使うようにしてください。
| hourStart := current | ||
| nextHour := current.Add(time.Hour) | ||
|
|
||
| playStart := logStart |
There was a problem hiding this comment.
playStartとplayEndは、そのhourの間のプレイ開始・終了時刻を表すと思うのですが、hour要素が無くてわかりにくい気がします。hourlyPlayStartとhourlyPlayEndのような感じにするのはどうでしょうか
| gameStats.playTime += playDuration | ||
| gameStatsMap[playLog.GameID] = gameStats | ||
|
|
||
| current := time.Date(logStart.Year(), logStart.Month(), logStart.Day(), logStart.Hour(), 0, 0, 0, logStart.Location()) |
There was a problem hiding this comment.
time.Time型のcurrentは現在時刻感が非常に強くなってしまうので、違う名前にしたいです。hourlyRangeStartとかはどうでしょうか
| current := time.Date(logStart.Year(), logStart.Month(), logStart.Day(), logStart.Hour(), 0, 0, 0, logStart.Location()) | ||
| isFirstHour := true | ||
|
|
||
| for current.Before(logEnd) { |
There was a problem hiding this comment.
forの3つ並べるやつでやると、横に長くなっちゃうけど、意味がとらえやすいのとhourlyRangeStartのスコープが狭まっていい感じになると思います。
for hourlyRangeStart := time.Date(...); hourlyRangeStart.Before(logEnd); hourlyRangeStart = hourlyRangeStart.Add(time.Hour) {
...
}| err = db.Model(&schema.GamePlayLogTable{}). | ||
| Where("edition_id = ?", uuid.UUID(editionID)). | ||
| Where("start_time <= ?", end). | ||
| Where("(end_time >= ? OR end_time IS NULL)", start). |
There was a problem hiding this comment.
仕様が明確になってなかった気がしますが、ここの不等号のイコールはいらないかもです。
仕様としては、重複が発生してしまうので、[start,end)の範囲でいいんじゃないかなと思っています。
| return | ||
| } | ||
|
|
||
| require.NoError(t, err) |
There was a problem hiding this comment.
テストの結果としてエラー出ないことを知りたいので、assertにしてください。
| require.NoError(t, err) | |
| assert.NoError(t, err) |
| assert.ErrorIs(t, err, testCase.err) | ||
| assert.Error(t, err) | ||
| if testCase.err != nil { | ||
| assert.Equal(t, testCase.err, err) |
There was a problem hiding this comment.
エラーの比較にはEqualではなくErrorsIsを使うようにしてください
| assert.Equal(t, testCase.err, err) | |
| assert.ErrorsIs(t, testCase.err, err) |
こちらは自分の勘違いでした(仕様をごっちゃにしていました)。すみません:pray: |
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
ありがとう。issueもcloseしちゃってください
User description
複数の時間台にまたがる(例14時台,15時台にまたがる(14:15-15:20))様なものを分離して記録するロジックをうまくSQLで表現できず，取ってきてGoでごちゃごちゃ処理する様な形になってしまいました。その点について，うまいやり方があればご教示いただきたいです:pray:
(#1393 をみたのですが，これは一方の時間帯のログとして処理していそう？)
PR Type
Enhancement, Tests
Description
Edition別プレイ統計の実装
進行中ログの時間分割集計
存在確認と未検出時のエラー対応
包括的な統計テスト追加
Diagram Walkthrough
File Walkthrough
v2_game_play_log.go
Edition別プレイ統計取得処理を実装src/repository/gorm2/v2_game_play_log.go
v2_game_play_log_test.go
Edition別統計の包括テストを追加src/repository/gorm2/v2_game_play_log_test.go