Skip to content

Review, Improvement Ideas and Possible Alternatives #145

@duongphuhiep

Description

@duongphuhiep

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:

  1. 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)?

  2. 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 record and with keywords, 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.
  3. 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  
        }  
    };
  4. Naming conventions: The project appears to follow Java naming conventions rather than C# conventions. (In addition, IRESTClient is 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.

  5. Instead of maintaining a custom REST client, why not consider established and widely adopted libraries such as RestSharp, Refit, or Flurl.Http?

  6. Since IRESTClient has only a single implementation (DefaultRESTClient), it may be simpler to remove the abstraction and work directly with DefaultRESTClient.

  7. 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/await pattern 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions