Refactored Requester #480
Refactored Requester #480
Conversation
Intention is to seperate the dependency injection friendly class from the singleton code
There was a problem hiding this comment.
Looks very good 👍 I like all of your changes, but implementing it in the whole project might take a while. I propose we move these changes into a new branch until we have a new working implementation of the RateLimitedRequester, which matches this pattern. When this is done, we could start pushing this into the develop branch.
| public async Task<T> DeserializeToAsync<T>(HttpResponseMessage message) | ||
| { | ||
| var json = await message.Content.ReadAsStringAsync(); | ||
| return JsonConvert.DeserializeObject<T>(json); |
There was a problem hiding this comment.
Couldn't you run this in a task?
There was a problem hiding this comment.
I did not see the benefit. After a bit of googling I have changed my mind and will put run it in one. Thanks
| namespace RiotSharp | ||
| { | ||
| public class StatusRiotApi : IStatusRiotApi | ||
| public partial class StatusRiotApi : IStatusRiotApi |
There was a problem hiding this comment.
Why do you want to split this into two partial classes? I don't think this is really needed.
There was a problem hiding this comment.
I had considered placing the second partial class in a separate file beside the original named "StatusRiotApiSingleton.cs."
I preferred separating the logic of the class from the method of creating/getting an instance. But maybe partial classes aren't the best way of doing this? Does splitting them up not provide some benefit in readability?
There was a problem hiding this comment.
You can split them up into two different files. However, spliting the class into partial classes inside the same file doesn't really provide a benefit in my eyes. You can achieve the same effect with regions, which in my opinion, is cleaner.
| { | ||
| public interface IRequestCreator | ||
| { | ||
| HttpRequestMessage CreateGetRequest(Region region, string apiEndpoint, List<string> addedArguments, bool useHttps = true); |
There was a problem hiding this comment.
Here might be needed functions for creating PUT and POST requests. They will be needed for the tournament endpoint.
There was a problem hiding this comment.
You're right. I hadn't written them yet since this only covered the standard requester which is only used by the status api. I'll include it in the branch you suggested.
2 participants
I have rewritten a version of the standard requester by attempting to separate its responsibilities into new classes. Benefits are improved testability (httpclient can be injected) and I think it's cleaner.
I'd appreciate feedback. If positive I'll continue and attempt to replace the other requesters.