Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor messages #9

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+45 −51
Open

refactor messages #9

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45313dc
refactor(messages): rename message types
edge2992 Apr 30, 2023
1d48d87
refactor: rename messages.PlayerLeft to messages.RoomLeft & remove un…
edge2992 Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ func (c *Client) handleMessages() {

func (c *Client) processMessage(message *messages.Message) {
switch message.Type {
case messages.CreateRoom:
case messages.RoomCreated:
c.handleCreateRoomMessage(message)
case messages.JoinRoom:
case messages.RoomJoined:
c.handleJoinRoomMessage(message)
case messages.PlayerLeft:
c.handleLeaveRoomMessage(message)
Expand Down Expand Up @@ -124,15 +124,15 @@ func (c *Client) Run() {

func (c *Client) CreateRoom(roomName string) {
message := messages.Message{
Type: messages.CreateRoom,
Type: messages.RequestCreateRoom,
RoomID: roomName, // ignored by server
}
c.connection.Encode(&message)
}

func (c *Client) requestRoomList() []string {
message := messages.Message{
Type: messages.ListRooms,
Type: messages.RequestRoomList,
}
c.sendMessage(&message)

Expand All @@ -144,7 +144,7 @@ func (c *Client) requestRoomList() []string {

func (c *Client) JoinRoom(roomID string) {
message := messages.Message{
Type: messages.JoinRoom,
Type: messages.RequestJoinRoom,
RoomID: roomID,
}
c.sendMessage(&message)
Expand Down Expand Up @@ -190,7 +190,7 @@ func (c *Client) handleTurnStarted(roomID string) {
log.Println("It's your turn!")
c.turnFlag = true
message := messages.Message{
Type: messages.DiceRolled,
Type: messages.RequestRollDice,
RoomID: roomID,
}
c.sendMessage(&message)
Expand All @@ -211,7 +211,7 @@ func (c *Client) reRollDice(dice []game.Dice, roomID string) {
selectedIndices := c.ioHandler.GetPlayerHoldInput(dice)
game.HoldDice(dice, selectedIndices)
message := messages.Message{
Type: messages.RerollDice,
Type: messages.RequestRerollDice,
RoomID: roomID,
Dice: dice,
}
Expand All @@ -221,7 +221,7 @@ func (c *Client) reRollDice(dice []game.Dice, roomID string) {
func (c *Client) chooseCategory(player *game.PlayerInfo, dice []game.Dice, roomID string) {
category := c.ioHandler.ChooseCategory(player, dice)
message := messages.Message{
Type: messages.ChooseCategory,
Type: messages.RequestChooseCategory,
RoomID: roomID,
Player: player,
Category: category,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

以下は、ローカルにあるGoコードですね。いくつかの問題や改善提案があります。

  • メッセージタイプについての命名方法にアンバランスがあります。次のような例があります。
    • messages.CreateRoom を messages.RoomCreated に変更する必要があります。
    • messages.JoinRoom を messages.RoomJoined に変更する必要があります。
    • messages.DiceRolled を messages.RequestRollDice に変更する必要があります。
    • messages.RerollDice を messages.RequestRerollDice に変更する必要があります。
    • messages.ChooseCategory を messages.RequestChooseCategory に変更する必要があります。
  • sendMessage()メソッドの使用の不一致があります。requestRoomList()とすべきですが、sendMessage()が使われています。
  • CreateRoom()メソッドでは、「messages.CreateRoom」を送信しますが、サーバーでは無視され、その代わりにroomNameを使用する必要があります。

以上、ご参考までに。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このパッチのコードレビューを行います。

  • messages.CreateRoom、messages.JoinRoom、messages.PlayerLeft のそれぞれが、messages.RoomCreated、messages.RoomJoined、messages.RoomLeft に変更されました。適切な名前に変更されたようです。
  • messages.ListRooms が messages.RequestRoomList に変更されました。同じ意味ですが、メッセージの種類をリクエスト/レスポンスの形式に合わせたようです。
  • messages.DiceRolled、messages.RerollDice、messages.ChooseCategory が、messages.RequestRollDice、messages.RequestRerollDice、messages.RequestChooseCategory に変更されました。これらもリクエスト/レスポンスの形式に合わせた変更であると思われます。

改善点としては、requestRoomList()関数での返り値が設定されていないことが挙げられます。また、各メッセージに対する応答用の構造体を導入することで、メッセージ間の整合性を確認できるようになる可能性があります。

Expand Down
12 changes: 6 additions & 6 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func TestCreateRoom(t *testing.T) {

msg := conn.TopEncodedMessage().(*messages.Message)

if msg.Type != messages.CreateRoom {
if msg.Type != messages.RequestCreateRoom {
t.Fatalf("Expected message type to be CreateRoom, got %d", msg.Type)
}
}
Expand All @@ -41,7 +41,7 @@ func TestJoinRoom(t *testing.T) {

msg := conn.TopEncodedMessage().(*messages.Message)

if msg.Type != messages.JoinRoom {
if msg.Type != messages.RequestJoinRoom {
t.Fatalf("Expected message type to be JoinRoom, got %d", msg.Type)
}
}
Expand Down Expand Up @@ -99,7 +99,7 @@ func TestHandleTurnStarted(t *testing.T) {
t.Fatal("Expected messages.Message type")
}

if msg.Type != messages.DiceRolled {
if msg.Type != messages.RequestRollDice {
t.Fatalf("Expected message type to be DiceRolled, got %d", msg.Type)
}
}
Expand Down Expand Up @@ -144,7 +144,7 @@ func TestHandleDiceRolled(t *testing.T) {
t.Fatal("Expected messages.Message type")
}

if msg.Type != messages.RerollDice {
if msg.Type != messages.RequestRerollDice {
t.Fatalf("Expected message type to be RerollDice, got %d", msg.Type)
}

Expand Down Expand Up @@ -182,7 +182,7 @@ func TestReRollDice(t *testing.T) {
t.Fatal("Expected messages.Message type")
}

if msg.Type != messages.RerollDice {
if msg.Type != messages.RequestRerollDice {
t.Fatalf("Expected message type to be RerollDice, got %d", msg.Type)
}

Expand Down Expand Up @@ -216,7 +216,7 @@ func TestChooseCategory(t *testing.T) {
t.Fatal("Expected messages.Message type")
}

if msg.Type != messages.ChooseCategory {
if msg.Type != messages.RequestChooseCategory {
t.Fatalf("Expected message type to be ChooseCategory, got %d", msg.Type)
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードパッチは単体テストを修正するためのものです、以下の改善提案とバグのリスクがあります。

  • RequestCreateRoom 、RequestJoinRoom などの「Request」と名付ける必要があるかどうかは不明ですが、メッセージタイプの明確さに役立つと思われます。
  • メッセージタイプの比較で関数内でハードコーディングされた文字列よりも messages.CreateRoom, messages.RequestCreateRoom などのconstを使用する方が55行目、77行目、100行目、126行目、155行目、181行目、208行目など、可読性が向上します。
  • conn.TopEncodedMessage()では、connにエラーがない限り、決してnilを返すことがなく、同様にnilとの比較は不要です。
  • コメントは欠落していますが、分離後の各テストが何をテストしているかを明確にするコメントを作成した方がよいです。

以上のような改善策や注目点があげられます。

Expand Down
34 changes: 17 additions & 17 deletions messages/messages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,31 @@ import "yatzcli/game"
type MessageType int

const (
GameJoined MessageType = iota
GameLeft
ServerJoin MessageType = iota
WaitForPlayers
RoomFull
RoomCreated
RoomJoined
RoomLeft
GamePlayerJoined
GamePlayerLeft
GameStart
GameStarted
PlayerReady
PlayerJoined
PlayerJoinedRoom
GameOver
PlayerLeft
DiceRolled
RerollDice
TurnStarted
TurnPlayed
ChooseCategory
UpdateScorecard
RollDice
UpdateGameState
GameOver
CreateRoom
JoinRoom
LeaveRoom
ListRooms
ListRoomsResponse
WaitForPlayers
RoomFull
ServerJoin
RequestRollDice
RequestRerollDice
RequestChooseCategory
RequestCreateRoom
RequestJoinRoom
RequestLeaveRoom
RequestRoomList
RoomListResponse
)

type Message struct {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードパッチは、MessageTypeを使用してメッセージ型を追加し、それに基づいてMessage構造体を定義するようです。コードの変更自体はシンプルで明らかなバグリスクはありません。ただし、いくつかの改善点があります。

  • 変更した MessageType を適切な名称に修正し、各メッセージの意図を示す名前を付けることでコードの読みやすさを向上させることができます。
  • GameJoined や GameLeft のような、現在使われていない MessageType のために予約された名前を解放することもできます。
  • メッセージ型に新しい値を追加する場合は、TCP/IPの設計原則に従い、バージョン管理を考慮する必要があるかもしれません。

以上のような変更を行うことによって、コードの可読性と保守性が向上する可能性があります。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードパッチは、MessageTypeに一部の要素を追加し、他の要素を変更しています。しかし、 Message構造体に対する変更はありませんでした。

バグリスクがあるかどうかは自明ではありませんが、 Message構造体に何らかの変更がないため、このパッチは機能の改善を提供していません。Code Reviewとしては、あなたが書くべきよりも多くの説明を提供することができます。また、リファクタリングやパフォーマンスの向上を提案する方法もあるかもしれませんが、それについての情報は提供されませんでした。

Expand Down
6 changes: 3 additions & 3 deletions server/gameplay_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,11 @@ func (gpc *GamePlayController) HandleMessage(message *messages.Message, player *
switch message.Type {
case messages.TurnStarted:
gpc.StartTurn(message.RoomID, player)
case messages.DiceRolled:
case messages.RequestRollDice:
gpc.RollDice(message.RoomID, player)
case messages.RerollDice:
case messages.RequestRerollDice:
gpc.RerollDice(message.RoomID, player, message.Dice)
case messages.ChooseCategory:
case messages.RequestChooseCategory:
gpc.ChooseScoreCategory(message.RoomID, player, message.Category)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードパッチは、messagesパッケージに含まれる様々なメッセージタイプに対応するGamePlayControllerの関数をアップデートしています。メッセージタイプの名称が変更されており、全て "Request" を含んでいます。
バグのリスクを特定することはできませんが、以下の改善提案があります：

  1. メッセージタイプに説明的な名前を付けて、より認識性が高いものにします。例えば、"DiceRolled" の代わりに "RollDiceResponse" や "RolledDiceResult" といった名称を使用することで、意図がより明確になるでしょう。

  2. コメントを追加して、各関数の目的や動作をより説明的にすることができます。

  3. 直接的なバグは特定できませんが、関数内のそれぞれの引数が適切な型であることを確認します。

以上の点を考慮することで、より読みやすくかつエラーの少ないコードを書くことができます。

16 changes: 8 additions & 8 deletions server/room_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (rc *RoomController) CreateRoom(player *Player) {
rc.roomManager.JoinRoom(roomID, player)

message := messages.Message{
Type: messages.CreateRoom,
Type: messages.RoomCreated,
Player: player.PlayerInfo(),
RoomID: roomID,
}
Expand Down Expand Up @@ -91,7 +91,7 @@ func (rc *RoomController) addPlayerToRoom(roomID string, player *Player) {

func (rc *RoomController) notifyPlayerJoinedRoomToOthers(room *Room, player *Player) {
message := messages.Message{
Type: messages.JoinRoom,
Type: messages.RoomJoined,
Player: player.PlayerInfo(),
RoomID: room.ID,
}
Expand All @@ -107,7 +107,7 @@ func (rc *RoomController) ListRooms(player *Player) {
}

message := messages.Message{
Type: messages.ListRoomsResponse,
Type: messages.RoomListResponse,
Player: player.PlayerInfo(),
RoomList: roomList,
}
Expand All @@ -122,7 +122,7 @@ func (rc *RoomController) LeaveRoom(roomID string, player *Player) {
}

message := messages.Message{
Type: messages.LeaveRoom,
Type: messages.RoomLeft,
Player: player.PlayerInfo(),
RoomID: roomID,
}
Expand All @@ -138,13 +138,13 @@ func (rc *RoomController) LeaveRoom(roomID string, player *Player) {

func (rc *RoomController) HandleMessage(message *messages.Message, player *Player) {
switch message.Type {
case messages.CreateRoom:
case messages.RequestCreateRoom:
rc.CreateRoom(player)
case messages.JoinRoom:
case messages.RequestJoinRoom:
rc.JoinRoom(message.RoomID, player)
case messages.ListRooms:
case messages.RequestRoomList:
rc.ListRooms(player)
case messages.LeaveRoom:
case messages.RequestLeaveRoom:
rc.LeaveRoom(message.RoomID, player)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードパッチは、メッセージ型定数(messages)での変更と関数名の微調整を含みます。このパッチ自体には問題はなさそうで、リファクタリングされていますが、他のソースコードとの互換性の観点から、機能の変更や不足がないか確認が必要です。

改善提案：

  • メッセージタイプをより明確にするために「Request」や「Response」といった単語を追加することができます。
  • 一部の関数名であいまいな表現が使われており、より具体的な名前にすることが望ましいかもしれません。例えば、「ListRooms」は「GetRoomList」、「LeaveRoom」は「ExitRoom」などです。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request, responseの命名規則を採用します

6 changes: 3 additions & 3 deletions server/room_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestRoomController_CreateRoom(t *testing.T) {
}

msg := mockConn.EncodedMessages[0].(*messages.Message)
if msg.Type != messages.CreateRoom {
if msg.Type != messages.RoomCreated {
t.Error("Expected message to be CreateRoom")
}

Expand Down Expand Up @@ -105,7 +105,7 @@ func TestRoomController_ListRooms(t *testing.T) {
t.Fatalf("Expected message, got %T", mockConn.EncodedMessages[0])
}

if message.Type != messages.ListRoomsResponse {
if message.Type != messages.RoomListResponse {
t.Errorf("Expected message type ListRoomsResponse, got %d", message.Type)
}

Expand Down Expand Up @@ -150,7 +150,7 @@ func TestRoomController_LeaveRoom(t *testing.T) {

msg := mockConn.TopEncodedMessage().(*messages.Message)

if msg.Type != messages.LeaveRoom {
if msg.Type != messages.RoomLeft {
t.Error("Expected message to be LeaveRoom")
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このパッチのコードレビューを行います。

48行目のif文で、Messages型の変数msgがmessages.RoomCreatedと一致するか確認しています。この場合、messages.CreateRoomと比較されるべきです。また、47行目にmsg変数の宣言が欠落しているため、その修正が必要です。

105行目のif文では、message.Typeがmessages.RoomListResponseと一致するかどうかを確認しています。これは正しい比較です。

TestRoomController_LeaveRoom ()関数の150行目のif文で、msg.Typeがmessages.RoomLeftという値であることを確認しています。これは正しい比較です。

以上から、欠陥や問題は見つかりませんでした。しかし、47行目の修正が必要です。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードのパッチでは、以下のような変更がされています。

  • CreateRoom メッセージの代わりに RoomCreated メッセージを期待するようになっている。
  • ListRoomsResponse メッセージの代わりに RoomListResponse メッセージを期待するようになっている。
  • LeaveRoom メッセージの代わりに RoomLeft メッセージを期待するようになっている。

この変更によって、テストで期待されるメッセージと実際のメッセージが一致しなかった場合にエラーメッセージが出力されるようになりました。

このコードに問題があるかどうか、また改善提案があるかどうかは、与えられたコード断片自体からは決定できません。全体的にどのように使われているのか、その文脈に基づいて評価する必要があります。ただし、与えられた部分自体は、メッセージタイプの比較を正しく行っているように見えます。

Expand Down