refactor messages #9
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -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, | |||
There was a problem hiding this comment.
以下は、ローカルにある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を使用する必要があります。
以上、ご参考までに。
| @@ -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) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
このコードパッチは単体テストを修正するためのものです、以下の改善提案とバグのリスクがあります。
RequestCreateRoom、RequestJoinRoomなどの「Request」と名付ける必要があるかどうかは不明ですが、メッセージタイプの明確さに役立つと思われます。- メッセージタイプの比較で関数内でハードコーディングされた文字列よりも
messages.CreateRoom,messages.RequestCreateRoom などのconstを使用する方が55行目、77行目、100行目、126行目、155行目、181行目、208行目など、可読性が向上します。 conn.TopEncodedMessage()では、connにエラーがない限り、決してnilを返すことがなく、同様にnilとの比較は不要です。- コメントは欠落していますが、分離後の各テストが何をテストしているかを明確にするコメントを作成した方がよいです。
以上のような改善策や注目点があげられます。
| RequestJoinRoom | ||
| RequestLeaveRoom | ||
| RequestRoomList | ||
| RoomListResponse | ||
| ) | ||
|
|
||
| type Message struct { |
There was a problem hiding this comment.
このコードパッチは、MessageTypeを使用してメッセージ型を追加し、それに基づいてMessage構造体を定義するようです。コードの変更自体はシンプルで明らかなバグリスクはありません。ただし、いくつかの改善点があります。
- 変更した
MessageTypeを適切な名称に修正し、各メッセージの意図を示す名前を付けることでコードの読みやすさを向上させることができます。 GameJoinedやGameLeftのような、現在使われていないMessageTypeのために予約された名前を解放することもできます。- メッセージ型に新しい値を追加する場合は、TCP/IPの設計原則に従い、バージョン管理を考慮する必要があるかもしれません。
以上のような変更を行うことによって、コードの可読性と保守性が向上する可能性があります。
| gpc.RerollDice(message.RoomID, player, message.Dice) | ||
| case messages.ChooseCategory: | ||
| case messages.RequestChooseCategory: | ||
| gpc.ChooseScoreCategory(message.RoomID, player, message.Category) | ||
| } | ||
| } |
There was a problem hiding this comment.
このコードパッチは、messagesパッケージに含まれる様々なメッセージタイプに対応するGamePlayControllerの関数をアップデートしています。メッセージタイプの名称が変更されており、全て "Request" を含んでいます。
バグのリスクを特定することはできませんが、以下の改善提案があります：
-
メッセージタイプに説明的な名前を付けて、より認識性が高いものにします。例えば、"DiceRolled" の代わりに "RollDiceResponse" や "RolledDiceResult" といった名称を使用することで、意図がより明確になるでしょう。
-
コメントを追加して、各関数の目的や動作をより説明的にすることができます。
-
直接的なバグは特定できませんが、関数内のそれぞれの引数が適切な型であることを確認します。
以上の点を考慮することで、より読みやすくかつエラーの少ないコードを書くことができます。
| rc.ListRooms(player) | ||
| case messages.LeaveRoom: | ||
| case messages.RequestLeaveRoom: | ||
| rc.LeaveRoom(message.RoomID, player) | ||
| } | ||
| } |
There was a problem hiding this comment.
このコードパッチは、メッセージ型定数(messages)での変更と関数名の微調整を含みます。このパッチ自体には問題はなさそうで、リファクタリングされていますが、他のソースコードとの互換性の観点から、機能の変更や不足がないか確認が必要です。
改善提案：
- メッセージタイプをより明確にするために「Request」や「Response」といった単語を追加することができます。
- 一部の関数名であいまいな表現が使われており、より具体的な名前にすることが望ましいかもしれません。例えば、「ListRooms」は「GetRoomList」、「LeaveRoom」は「ExitRoom」などです。
There was a problem hiding this comment.
Request, responseの命名規則を採用します
| @@ -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") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
このパッチのコードレビューを行います。
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行目の修正が必要です。
…used message types
| @@ -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, | |||
There was a problem hiding this comment.
このパッチのコードレビューを行います。
- 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()関数での返り値が設定されていないことが挙げられます。また、各メッセージに対する応答用の構造体を導入することで、メッセージ間の整合性を確認できるようになる可能性があります。
| RequestJoinRoom | ||
| RequestLeaveRoom | ||
| RequestRoomList | ||
| RoomListResponse | ||
| ) | ||
|
|
||
| type Message struct { |
There was a problem hiding this comment.
このコードパッチは、MessageTypeに一部の要素を追加し、他の要素を変更しています。しかし、 Message構造体に対する変更はありませんでした。
バグリスクがあるかどうかは自明ではありませんが、 Message構造体に何らかの変更がないため、このパッチは機能の改善を提供していません。Code Reviewとしては、あなたが書くべきよりも多くの説明を提供することができます。また、リファクタリングやパフォーマンスの向上を提案する方法もあるかもしれませんが、それについての情報は提供されませんでした。
| @@ -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") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
このコードのパッチでは、以下のような変更がされています。
- CreateRoom メッセージの代わりに RoomCreated メッセージを期待するようになっている。
- ListRoomsResponse メッセージの代わりに RoomListResponse メッセージを期待するようになっている。
- LeaveRoom メッセージの代わりに RoomLeft メッセージを期待するようになっている。
この変更によって、テストで期待されるメッセージと実際のメッセージが一致しなかった場合にエラーメッセージが出力されるようになりました。
このコードに問題があるかどうか、また改善提案があるかどうかは、与えられたコード断片自体からは決定できません。全体的にどのように使われているのか、その文脈に基づいて評価する必要があります。ただし、与えられた部分自体は、メッセージタイプの比較を正しく行っているように見えます。
No description provided.