-
Notifications
You must be signed in to change notification settings - Fork 13
Description
First of all, thank you to all the contributors for your work on this library. I’ve reviewed the code base and, with full respect, would like to share some observations and suggestions:
-
The library explicitly uses C# 7.3. Is there a particular reason for not updating to the latest available version of C# (currently C# 14)?
-
At first glance, the codebase (for example this file) appear to follow a Java-oriented style rather than C#.
- In modern C# (9+), we have the
recordandwithkeywords, which are especially well-suited for DTOs and domain entities. - Generally, C# favors properties over public fields, as this provides better encapsulation, flexibility, and consistency with common .NET conventions and frameworks.
- In modern C# (9+), we have the
-
The
with()methods on (every) entities seems to introduce unnecessary complexity and feels more aligned with Java patterns than C#. For example:var request = new RegistrationRequest() .with(request => request.user = new User() .with(user => user.email = Guid.NewGuid() + "@example.com") .with(user => user.password = "password")) .with(request => request.registration = new UserRegistration() .with(registration => registration.applicationId = Guid.Empty));
In idiomatic C#, the same can be achieved more simply using object initializers:
var request = new RegistrationRequest { user = new User { email = Guid.NewGuid() + "@example.com"; password = "password"; }, registration = new UserRegistration() { applicationId = Guid.Empty } };
-
Naming conventions: The project appears to follow Java naming conventions rather than C# conventions. (In addition,
IRESTClientis defined as an abstract class rather than an interface, despite the “I” prefix. This is unusual in C# and even uncommon in Java. Adopting standard .NET naming conventions would make the library feel more idiomatic and approachable for C# developers. -
Instead of maintaining a custom REST client, why not consider established and widely adopted libraries such as RestSharp, Refit, or Flurl.Http?
-
Since
IRESTClienthas only a single implementation (DefaultRESTClient), it may be simpler to remove the abstraction and work directly withDefaultRESTClient. -
Regarding this goAsync() method
public override Task<ClientResponse<T>> goAsync<T>() { return baseRequest().ContinueWith(task => { var result = task.Result; JsonConvert.DeserializeObject<T>(result.Content.ReadAsStringAsync().Result, SerializerSettings); ... }) }
In C#, the
async/awaitpattern provides a clearer and safer approach:public override async Task<ClientResponse<T>> goAsync<T>() { var result = await baseRequest().ConfigureAwait(false); JsonConvert.DeserializeObject<T>( await result.Content.ReadAsStringAsync().ConfigureAwait(false), SerializerSettings); ... }
Beyond syntax, the current Java-like codes is problematic, let's hear ChatGPT reviewing this goAsync() method
Other alternatives
After reviewing the code, I explored alternatives based on the OpenAPI specs published by FusionAuth. Using Kiota, I was able to generate a working C# client and integrate it into a Blazor application. While my experiment was only a “Hello World” test, it showed promise.
If Kiota feels too heavy, NSwag is another strong option and can generate lighter and more straightforward C# clients.
That said, the OpenAPI specs themselves have issues, so generated clients (Kiota or NSwag) may not be perfect out of the box.