Skip to content

Commit 635157c

Browse files
Copilotstephentoub
andcommitted
Refactor Read method based on code review feedback
- Use switch expression on (HasId, HasMethod) tuple to reduce branching - Replace null check exceptions with Debug.Assert since HasError/HasMethod flags are only set when values are non-null - Simplify control flow for better performance and readability - All 1035 tests still pass Co-authored-by: Scooletz <scooletz@users.noreply.github.com> Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com> Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
1 parent 36d2e74 commit 635157c

1 file changed

Lines changed: 40 additions & 55 deletions

File tree

src/ModelContextProtocol.Core/Protocol/JsonRpcMessage.cs

Lines changed: 40 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -85,73 +85,58 @@ public sealed class Converter : JsonConverter<JsonRpcMessage>
8585
throw new JsonException("Invalid or missing jsonrpc version");
8686
}
8787

88-
// Messages with an id but no method are responses
89-
if (union.HasId && !union.HasMethod)
88+
// Determine message type based on presence of id and method properties
89+
switch ((union.HasId, union.HasMethod))
9090
{
91-
// Messages with an error property are error responses
92-
if (union.HasError)
93-
{
94-
if (union.Error is null)
95-
{
96-
throw new JsonException("Error property cannot be null");
97-
}
98-
99-
return new JsonRpcError
91+
case (true, true):
92+
// Messages with both method and id are requests
93+
Debug.Assert(union.Method is not null, "HasMethod should only be true when Method is non-null");
94+
return new JsonRpcRequest
10095
{
10196
JsonRpc = union.JsonRpc,
10297
Id = union.Id,
103-
Error = union.Error
98+
Method = union.Method!,
99+
Params = union.Params
104100
};
105-
}
106101

107-
// Messages with a result property are success responses
108-
if (union.HasResult)
109-
{
110-
return new JsonRpcResponse
102+
case (true, false):
103+
// Messages with an id but no method are responses
104+
if (union.HasError)
111105
{
112-
JsonRpc = union.JsonRpc,
113-
Id = union.Id,
114-
Result = union.Result
115-
};
116-
}
117-
118-
throw new JsonException("Response must have either result or error");
119-
}
106+
Debug.Assert(union.Error is not null, "HasError should only be true when Error is non-null");
107+
return new JsonRpcError
108+
{
109+
JsonRpc = union.JsonRpc,
110+
Id = union.Id,
111+
Error = union.Error!
112+
};
113+
}
120114

121-
// Messages with a method but no id are notifications
122-
if (union.HasMethod && !union.HasId)
123-
{
124-
if (union.Method is null)
125-
{
126-
throw new JsonException("Method property cannot be null");
127-
}
115+
if (union.HasResult)
116+
{
117+
return new JsonRpcResponse
118+
{
119+
JsonRpc = union.JsonRpc,
120+
Id = union.Id,
121+
Result = union.Result
122+
};
123+
}
128124

129-
return new JsonRpcNotification
130-
{
131-
JsonRpc = union.JsonRpc,
132-
Method = union.Method,
133-
Params = union.Params
134-
};
135-
}
125+
throw new JsonException("Response must have either result or error");
136126

137-
// Messages with both method and id are requests
138-
if (union.HasMethod && union.HasId)
139-
{
140-
if (union.Method is null)
141-
{
142-
throw new JsonException("Method property cannot be null");
143-
}
127+
case (false, true):
128+
// Messages with a method but no id are notifications
129+
Debug.Assert(union.Method is not null, "HasMethod should only be true when Method is non-null");
130+
return new JsonRpcNotification
131+
{
132+
JsonRpc = union.JsonRpc,
133+
Method = union.Method!,
134+
Params = union.Params
135+
};
144136

145-
return new JsonRpcRequest
146-
{
147-
JsonRpc = union.JsonRpc,
148-
Id = union.Id,
149-
Method = union.Method,
150-
Params = union.Params
151-
};
137+
default:
138+
throw new JsonException("Invalid JSON-RPC message format");
152139
}
153-
154-
throw new JsonException("Invalid JSON-RPC message format");
155140
}
156141

157142
/// <inheritdoc/>

0 commit comments

Comments
 (0)