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

API Client refactor #17

Merged
merged 11 commits into from
Aug 28, 2016
Merged

API Client refactor #17

merged 11 commits into from
Aug 28, 2016

Conversation

robertmclaws
Copy link

@robertmclaws robertmclaws commented Aug 22, 2016

  • Broke out all RPC clients into interfaces for testability.
  • Renamed the internal client classes to make a bit more sense.
  • Stubbed out a possible refactor of LoginClient.SetServer that would eliminate some duplicate requests.

NOTE: Still WIP.

- Broke out all RPC clients into interfaces for testability.
- Renamed the internal client classes to make a bit more sense.
- Stubbed out a possible refactor of LoginClient.SetServer that would
eliminate some duplicate requests.
@review-ninja
Copy link

ReviewNinja

@robertmclaws
Copy link
Author

@WallyCZ, would love your opinion on how to refactor SetServer to be able to return the first set of data w/o needing a subsequent set of requests.

@WallyCZ
Copy link
Contributor

WallyCZ commented Aug 22, 2016

Currently it seems that we don't need any SetServer method anymore, because it tries to simulate initial communication to server, but it's different anyway. Or at least it's implementation should be changed, communication starts with repeated single (not batched) GetPlayer until reply comes (delay between request is 0.5s).

@ST-Apps
Copy link
Contributor

ST-Apps commented Aug 22, 2016

I didn't read all the code but SetServer is needed to set the first RPC uri when launching the app.

@WallyCZ
Copy link
Contributor

WallyCZ commented Aug 22, 2016

Ok so we need it, but we have to change it :)

@robertmclaws
Copy link
Author

SetServer does much more than that... it's called by DoLogin() to actually complete the Login request. It's sending GetPlayer, GetHatchedEggs, GetInventory, CheckAwardedBadges, and DownloadSettings, but not actually using any of that data. If we rewrite that code, we can actually use those results to eliminate duplicate calls.

@Chatfix Chatfix added the api label Aug 23, 2016
- Got rid of the HttpClientExtensions by moving Interfaces and Enums to
new files, and putting the other functions in PokemonClient where they
belong.
- Worked on new methods for generating batch requests.
- Initial build-out of the new request queueing mechanism.
@ST-Apps
Copy link
Contributor

ST-Apps commented Aug 24, 2016

It's shaping up nice, well done.
I'd say that this is also a good chance to simplify that Settings class.
We don't need both PTC and Google username/password, we only need one username, password + auth type.

@WallyCZ
Copy link
Contributor

WallyCZ commented Aug 24, 2016

It seems that last version 0.35 started to send at init phase GetPlayer and CheckChallenge (new feature) parallel and also CheckChallenge is added to batches (so now its 5 + 1 message in batch). We need strong mechanismus which will allow to control flow messages to match with original client.

@ST-Apps
Copy link
Contributor

ST-Apps commented Aug 24, 2016

@WallyCZ let's continue in #20

Robert McLaws added 9 commits August 25, 2016 10:07
- Implemented a pattern for each GameService to be responsible for
clearing its own state.
- Fixed a bug where the CancellationToken property was wrong.
- Started messing with an Event pattern for batch responses.
This was supposed to be in the last commit.
- TODO: Add everywhere else.
Found a new way to do Unit Tests.
Trying to get Unit Tests to work, but they don't, because Microsoft.
@ST-Apps ST-Apps merged commit 8eadaea into master Aug 28, 2016
@robertmclaws robertmclaws self-assigned this Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants