avivbiton / BlizzardApiReader

.Net Core library to handle Blizzard public APIs
MIT License
35 stars 36 forks source link

ApiConfiguration changes #50

Open avivbiton opened 5 years ago

avivbiton commented 5 years ago

In my last commit, I've moved the ApiConfiguration back to ApiReader. The reason is that I've felt ApiWebClient class should mostly operate on its own, I want to completely separate the ApiConfiguration class from the WebClient in the future.
Some issues caused by this was changing the configuration would create another instance of ApiWebClient which has some problems:

I've been thinking maybe adding a static class to handle settings like proxy and LifeTimeLimit (and later on caching and other settings that usually do not change often). The difference will be is that ApiConfiguration will be used only to hold properties that may change constantly while the application is still running. (Like Region and Locale, I may even remove ClientID and SecretID) and the static configuration class will hold settings that do not change often (or even at all) like ClientId and secretId, caching settings, proxy, pooledConnectionLifetime and more

This will shrink the responsibilities of the ApiConfiguration class (which seems to grow and will probably grow if it stays the way it is), will remove the dependency of the ApiWebClient from the ApiConfiguration, allow more freedom to change and modify both classes, remove the need to refresh the instance or initialization of ApiWebClient every time the configuration has changed.

To sum it up: Reduce ApiConfiguration properties. Create a new static configuration class Separate ApiWebClient from ApiConfiguration

I'd love to hear your thoughts on this.

redmars27 commented 5 years ago

I would recommend going more within this route https://mcguirev10.com/2018/01/31/net-core-class-library-dependency-injection.html than with static classes.

avivbiton commented 5 years ago

DI is interesting but I am just not sure if we should include a third party library in a project that is supposed to act as a library by itself.

redmars27 commented 5 years ago

DotNet core and DI go hand by hand. To me if we use dotnet core we should not have any problem with Microsoft.Extensions.DependencyInjection

Or what are the problems you see?

This will allow even to decouple the HttpClient

avivbiton commented 5 years ago

If it is part of .net core then sure.
But what I am talking about is a static class that will read settings from a setting file and those settings will not change during runtime since changing in runtime is extremely unlikely and may cause things to break.

If we do use DI for other classes though, where should we use it? we already wrap HttpClient behind an interface.

redmars27 commented 5 years ago

Let me see if I can come with a proposal and then we can review it again. I will need some time as the next two weeks I am traveling but will work on this afterwards.

mabbotts9797 commented 5 years ago

@avivbiton Happy to help with this if no one jumps in, looking for a first issue to help out with as Im new to contributing to open source.

avivbiton commented 5 years ago

@mabbotts9797 You can, you may pick an easier issue if you wish, we have some open and you can create one yourself

mabbotts9797 commented 5 years ago

@avivbiton Cool, I will take a look.