やりたいことボードのusecase層以下を実装 #31

sana37 · 2020-09-23T05:34:12Z

Issue

概要

やりたいことボード（WishBoard）のusecase層以下を実装

タスク整理

実装
テスト
mock?の作成

sana37 · 2020-09-23T05:36:39Z

テストとかまだ作れてませんが、とりあえず実装までは済んだのでいったんレビューお願いします！

https://github.com/golang/go/wiki/CodeReviewComments#initialisms

akubi0w1

ざーっとコメントしましたー！何箇所か気になったところがあったので、みていただけると(ㅅ ;´꒳`;)
コード読みやすかったです、、、！(ﾟ∀ﾟ)

akubi0w1 · 2020-09-25T14:03:16Z

pkg/api/usecase/wishboard/interactor.go

+	GetWishBoard(ctx context.Context, wishBoardID int, authID string) (*wishboard.Entity, error)
+	UpdateTitle(ctx context.Context, wishBoardID int, title, authID string) error
+	UpdateBackgroundImage(ctx context.Context, wishBoardID int, backgroundImage []byte, authID string) error
+	DeleteWishBoard(ctx context.Context, wishBoardID int, authID string) error


ただの気づきです。。。
interactor内のメソッドの引数、確かにauthIDのがいいかもな...！わたしuserIDで実装してたけど、userIDってよく考えたら取れないのか...！！(　ﾟдﾟ)

akubi0w1 · 2020-09-25T14:06:48Z

pkg/api/usecase/wishboard/interactor.go

+
+func (i *interactor) CreateNewWishBoard(ctx context.Context, authID, title string, backgroundImage []byte) (*wishboard.Entity, error) {
+	// 空のタイトルは許容しない
+	if title == "" {


感想ですー！
この辺のバリデーションなのだけど、項目増えていくと膨れ上がっていくから、関数として出しちゃってもいいかも。
公開にする必要はないだろうから、validateTitle(title string) errorくらいな感じで。

pkg/api/usecase/wishboard/interactor.go

akubi0w1 · 2020-09-25T14:16:20Z

pkg/domain/service/wishboard/service.go

+	GetByPK(ctx context.Context, masterTx repository.MasterTx, wishBoardID int) (*wishboard.Entity, error)
+	GetByOwner(ctx context.Context, masterTx repository.MasterTx, userID int) (wishboard.EntitySlice, error)
+	GetByMember(ctx context.Context, masterTx repository.MasterTx, userID int) (wishboard.EntitySlice, error)
+	UserBelongs(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)


感想です！
boolが返ってくるメソッドの名前は、IsXXXのが明確でいいかな、と思ったりなど。。。
IsUserBelongedとかかな...英語力...(ㅅ ;´꒳`;)

なるほど…
IsUserMemberでいこうかと思う！

わざわざ主語書かなくてもいいだろうってことでIsMemberになりました！

akubi0w1 · 2020-09-25T14:31:48Z

pkg/domain/repository/userwishboard/repository.go

+
+type Repository interface {
+	Insert(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) error
+	SelectByUserIDAndWishBoardID(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)


感想です〜
ここのメソッド、boolが返ってくるんで、名前がIsXXXの方が落ち着くかも

たしかに変だよな〜
シンプルにExistsとかにしようかな

akubi0w1 · 2020-09-25T14:39:25Z

pkg/infrastructure/mysql/userwishboard/repository.go

+		return nil, werrors.FromConstant(err, werrors.ServerError)
+	}
+
+	bis := make([]int, 0, 4)


ここの、3つめの引数の4って固定値、特に決まった値ってないはずなので、無理にmakeしなくていいかも！
rowsから件数取れんから、bis := []int{}でいいかなーと思う！

akubi0w1 · 2020-09-25T14:42:19Z

pkg/infrastructure/mysql/wishboard/repository.go

+	}
+
+	// 現在時刻を取得
+	now := time.Now()


感想ですー
時刻の取得なんだけど、DBアクセス系の処理じゃないので、serviceとかでやった方がいいかも？

akubi0w1 · 2020-09-25T14:45:19Z

pkg/infrastructure/mysql/wishboard/repository.go

+		return nil, werrors.FromConstant(err, werrors.ServerError)
+	}
+
+	bs := make(wishboard.EntitySlice, 0, 4)


ここの3つめの引数、固定値じゃない方がよいかもですー！

以下、複数同じところがあるので、同じくって感じです！

akubi0w1 · 2020-09-25T14:47:19Z

pkg/infrastructure/mysql/wishboard/repository.go

+	if err != nil {
+		if err == sql.ErrNoRows {
+			// 見つからなかったから空リストを返す
+			return []*wishboard.Entity{}, nil


ここのreturnで返すの、nil, nilで良いと思う！

そういえばgoの配列型ってnilでも空配列みたいな使い方できるんだっけ
了解、nilにしておく〜

akubi0w1 · 2020-09-25T14:50:05Z

pkg/werrors/constants.go

+		ErrorCode:      http.StatusForbidden,
+		ErrorMessageJP: "ボードへのアクセスが許可されていません",
+		ErrorMessageEN: "you don't have access to wish_board",
+	}


ナイス定義...！٩( 'ω' )و

sana37 · 2020-09-26T06:51:07Z

修正しました！

- insertとdeleteの引数をEntityに

TakumaKurosawa

いったんコメントしました！

pkg/domain/service/wishboard/service.go

TakumaKurosawa · 2020-09-27T07:18:54Z

pkg/api/usecase/wishboard/interactor.go

+type Interactor interface {
+	CreateNewWishBoard(ctx context.Context, authID, title string, backgroundImage []byte) (*wishboard.Entity, error)
+	GetMyWishBoards(ctx context.Context, authID string) (wishboard.EntitySlice, error)
+	GetWishBoard(ctx context.Context, wishBoardID int, authID string) (*wishboard.Entity, error)


今回の実装では必要ないので消して欲しい！

TakumaKurosawa · 2020-09-27T07:23:21Z

pkg/domain/service/wishboard/service.go

+	GetByPK(ctx context.Context, masterTx repository.MasterTx, wishBoardID int) (*wishboard.Entity, error)
+	GetByOwner(ctx context.Context, masterTx repository.MasterTx, userID int) (wishboard.EntitySlice, error)
+	GetByMember(ctx context.Context, masterTx repository.MasterTx, userID int) (wishboard.EntitySlice, error)
+	IsUserMember(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)


Suggested change

IsUserMember(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)

IsMember(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)

TakumaKurosawa · 2020-09-27T07:25:56Z

pkg/domain/service/wishboard/service.go

+	// 現在時刻を取得
+	now := time.Now()
+
+	b := &wishboard.Entity{


bだとわかりづらいので、newBoardのように変数見ただけで何を表しているかわかるような名前にしてください！
他のところも同様！！

sana37 · 2020-09-27T08:16:31Z

指摘のあった部分、修正しました！

TakumaKurosawa

コメントした！
これが修正できたらまた見るのでレビュー依頼よろです！

TakumaKurosawa · 2020-09-27T08:31:33Z

pkg/api/usecase/wishboard/interactor.go

+func validateTitle(ctx context.Context, title string) error {
+	if title == "" {
+		err := errors.New("title is empty")
+		tlog.PrintErrorLogWithCtx(ctx, err)
+		return werrors.FromConstant(err, werrors.BadRequest)
+	}
+
+	return nil
+}


これくらいの行数であれば、Privateメソッドに切り出す必要ないかなー。
バリデーションはその場で何をしているのか確認したかったりもするし、よっぽどメソッド切り分けしないイメージ。

これはあくびとも話したんだけど、タイトルのバリデーション項目が増えたときのことを考えると
切り出した方がいいかな、って思った

@sana37 cc: @akubi0w1
増えたら増えた時に考えたいのと、validateTitleのなかでいろいろなバリデーションが隠蔽化されていると使う方も怖くなると思うし、ユースケースごとにバリデーションを使い分けたいとなった時に、謎のバリデーション関数2みたいなのが出来上がる危険性があるので、もし分けるとしても、 ngWordCheckとかisEmptyとかみたいな単位で一個一個分けて欲しいかな。

了解！切り出さない方向で修正するね

TakumaKurosawa · 2020-09-27T08:34:55Z

pkg/api/usecase/wishboard/interactor.go

+	if err := validateTitle(ctx, title); err != nil {
+		return nil, werrors.Stack(err)
+	}


privateメソッドに切り出さない方が良さそう。

pkg/api/usecase/wishboard/interactor.go

TakumaKurosawa · 2020-09-27T08:37:28Z

pkg/api/usecase/wishboard/interactor.go

+		// TODO: 招待URLの自動生成
+		inviteURL := "hoge"


uuidで自動生成するだけやと思うから、ここで実装しちゃっても良いかもね？w

あと、実装する場合は、 wishBoardService.CreateInviteURLみたいにService層の実装として欲しい！

service層で実装する了解！
一応、仕様がはっきり決まるまでは実装しないでおく！
あと思ったんだけど、grpcで招待URLってどうやって使うんだろう…？

あと思ったんだけど、grpcで招待URLってどうやって使うんだろう…？

招待を受け付けるエンドポイントをRESTで用意してあげる必要があるね！

pkg/api/usecase/wishboard/interactor.go

TakumaKurosawa · 2020-09-27T09:07:56Z

pkg/infrastructure/mysql/userwishboard/repository.go

+	row := tx.QueryRow(`
+		SELECT id FROM users_wish_boards WHERE user_id = ? AND wish_board_id = ?
+	`, userID, wishBoardID)
+
+	var i int
+	err = row.Scan(&i)
+	if err != nil {
+		if err == sql.ErrNoRows {
+			// 見つからなければfalseを返す
+			return false, nil
+		}
+		return false, werrors.FromConstant(err, werrors.ServerError)
+	}


結局プログラムでやるのであれば、Service層で判定して欲しいかなー。
もしくは、関数名通り、MySQLのExists関数を使ってあげるかやなー。
https://qiita.com/mtanabe/items/5b8171a9d3ad9e559783

Existsなんてものがあったのね！
便利そうだから使う！

TakumaKurosawa · 2020-09-27T09:09:17Z

pkg/infrastructure/mysql/userwishboard/repository.go

+	return true, nil
+}
+
+func (r *repositoryImpliment) SelectByUserID(ctx context.Context, masterTx repository.MasterTx, userID int) ([]int, error) {


SelectByUserIDという関数名から、 []intが返ってくるとは思わなくて驚きそうだから、再設計をしてみて！

目的語ちゃんと足しとくね

pkg/infrastructure/mysql/userwishboard/repository.go

pkg/infrastructure/mysql/wishboard/repository.go

- user_wish_boardでExists句を使用 - メソッド名の変更

sana37 added 11 commits September 19, 2020 19:36

wishboardのrepositoryを追加

6c21bd0

ドメイン層までの実装およびusers_wish_boards関連の追加

f5e3264

create, get系までの実装およびwishBoardService.Createへのリレーション関連の処理追加

c1c1a37

セミコロンは不要

380d7f6

repository.Insertの引数を変更

63a937a

time関連の修正

bfea40e

wish_boardのupdate, deleteを追加

e77a907

update系を追加

7ded141

ユーザのボード一覧について、所有一覧ではなく所属一覧へと修正

e8ab473

delete系を追加

f1f6d6e

コメントを追加

a53b1e0

sana37 requested review from TakumaKurosawa and akubi0w1 September 23, 2020 05:38

sana37 added the devlop 開発環境に関するタスク label Sep 23, 2020

Url -> URL へと変更

97048bf

https://github.com/golang/go/wiki/CodeReviewComments#initialisms

akubi0w1 requested changes Sep 25, 2020

View reviewed changes

sana37 added 5 commits September 26, 2020 13:30

タイトルのバリデーションを関数として切り出し

af285f7

命名を修正

4bcf2c3

配列生成部分を修正

651fca9

空リストを返したいときはnilでも同じ

7c0a566

時刻の取得をdomain層で行う

3f420b1

TakumaKurosawa assigned sana37 Sep 27, 2020

TakumaKurosawa and others added 3 commits September 27, 2020 14:55

Merge branch 'develop' into feat/3/implWishListUsecase

a7f6368

- datetimeにtime.Timeのポインタ型を使用

f040add

- insertとdeleteの引数をEntityに

コメントの修正

ea185d9

TakumaKurosawa requested changes Sep 27, 2020

View reviewed changes

TakumaKurosawa changed the base branch from develop to feat/3/base September 27, 2020 07:28

メソッド名を変更

72509ac

sana37 added 4 commits September 27, 2020 16:34

GetByOwnerを削除、GetByMemberをGetMyBoardsに変更

1a419a6

コメントは最低限必要なものに限る

27b7929

変数名をわかりやすく

cc13e1a

本プルリクの対象ではないメソッドを削除

f80c6d0

TakumaKurosawa requested changes Sep 27, 2020

View reviewed changes

sana37 added 5 commits September 27, 2020 18:46

変数名の修正

f0abdec

コーディングスタイルの統一

88962cd

エラーログメッセージを修正

471ac90

２点修正

f9dd192

- user_wish_boardでExists句を使用 - メソッド名の変更

空タイトル判定は別関数に切り出したりしない

d9759d2

	IsUserMember(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)
	IsMember(ctx context.Context, masterTx repository.MasterTx, userID, wishBoardID int) (bool, error)

やりたいことボードのusecase層以下を実装 #31

Are you sure you want to change the base?

やりたいことボードのusecase層以下を実装 #31

Uh oh!

Conversation

sana37 commented Sep 23, 2020 • edited by TakumaKurosawa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

概要

タスク整理

Uh oh!

sana37 commented Sep 23, 2020

Uh oh!

akubi0w1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sana37 commented Sep 26, 2020

Uh oh!

TakumaKurosawa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sana37 commented Sep 27, 2020

Uh oh!

TakumaKurosawa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sana37 commented Sep 23, 2020 •

edited by TakumaKurosawa

Loading