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.

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

Proposed changes for 2.0 #34

Open
Ruhrpottpatriot opened this issue Dec 7, 2015 · 0 comments
Open

Proposed changes for 2.0 #34

Ruhrpottpatriot opened this issue Dec 7, 2015 · 0 comments
Assignees
Milestone

Comments

@Ruhrpottpatriot
Copy link
Owner

Currently each of our repositories implements IRepository, which in turn implements all other repository interfaces we currently have. This adds some inflexibility to endpoints which do not allow for some operations. Solutions are:

  1. Leave IRepository as it is and add NotSupportedException to the repositories
  2. Refactor IRepository into smaller interfaces and implement these interfaces on a need basis.

I advocate 2, not only are empty interfaces considered code smell to some degree, but our current pattern also violates the Interface segregation principle of SOLID.

In addition to refactoring out current interfaces, we should do some housekeeping. Current methods should either be be renamed to already established methods in C# (i.e. All(), Single(), etc.), or be renamed to CRUD method names (i.e. Get()).

Furthermore we also have to discuss, whether our code should fail-silent or not. Personally I advocate for a fail-fast approach. Therefore we should translate any HTTP-Statuscode into proper exceptions. As a curtesy to the user, we also could add fail-silent methods to the repositories (much akin to the ...OrDefault methods in LINQ).

Obviously every repository method is executed asynchronous and synchronous methods have been removed completely, since they serve no purpose anymore.

The current v2 repository layout looks like the code below:

public class ColorService : ServiceBase<ColorPalette>, IDiscoverService<int>, IApiService<int, ColorPalette>, ILocalizable
{
    public ColorService(
        HttpClient httpClient,
        HttpResponseConverter responseConverter,
        ICache<ColorPalette> cache,
        IConverter<IEnumerable<int>, IEnumerable<int>> identifiersConverter,
        IConverter<ColorPaletteDTO, ColorPalette> colorConverter)
        : base(httpClient, responseConverter, cache)
    {}

    public CultureInfo Culture { get; set; }

    public Task<IEnumerable<int>> DiscoverAsync() {}

    public async Task<IEnumerable<int>> DiscoverAsync(CancellationToken cancellationToken) {}

    public Task<IEnumerable<ColorPalette>> GetAsync(CancellationToken cancellationToken) {}

    public Task<IEnumerable<ColorPalette>> GetAsync(CancellationToken cancellationToken, params int[] identifiers) {}

    public async Task<IEnumerable<ColorPalette>> GetAsync(IEnumerable<int> identifiers, CancellationToken cancellationToken) {}

    public async Task<ColorPalette> GetAsync(int identifier, CancellationToken cancellationToken) {}

    public async Task<IEnumerable<ColorPalette>> GetAsync(Func<ColorPalette, bool> selector, CancellationToken cancellationToken) {}
}

public abstract class ServiceBase<T>
{
    protected ServiceBase(HttpClient client, HttpResponseConverter responseConverter, ICache<T> cache)
    {
        this.Client = client;
        this.ResponseConverter = responseConverter;
        this.Cache = cache;
    }

    public HttpClient Client { get; }

    public HttpResponseConverter ResponseConverter { get; }

    public ICache<T> Cache { get; }

    protected IEnumerable<int> CalculatePageSizes(int queryCount)
    {
        if (queryCount <= 200)
        {
            return new List<int> { queryCount };
        }

        return new List<int> { 200 }.Concat(this.CalculatePageSizes(queryCount - 200));
    }
}

There's a question that pops into my mind for repositories with localized output. Should we deal with localization by setting a property on the repository, which sets the locale for all further requests until changed, or should we add a (optional) method parameters which only set the locale for the next request.
In my opinion the latter is the more sensible solution. The cache already does not care about the locale and stores everything. When accessing it, the user can either specify a locale or not. In the former case only results matching the locale are returned, in the latter case everything matching the filter is returned, regardless of the culture.

The HttpResponseConverter is a special convert, which does not inherit from IConverter<TIn, TOut>. It's ConvertAsync method accepts a HttpResponseMessage, an inner converter, an optional state and a Cancellation token. The method then deserializes the response into the appropriate data contract, before passing the results to the inner converter, which then converts the data contracts into actual objects.

@Ruhrpottpatriot Ruhrpottpatriot added this to the v2.0.0 milestone Feb 11, 2016
@Ruhrpottpatriot Ruhrpottpatriot self-assigned this Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant