Skip to content
This repository was archived by the owner on Aug 7, 2021. It is now read-only.

Conversation

@MiniClem
Copy link

#43
I've got to change methods accessors from Request and move classes to new packages.
Actually I'm just doing it the easy way, breaking each endpoints from the AsynchronousRequest class to each new endpoint classes.
There is something I would like to know, I made an example in the code, for example if you got that endpoint /v2/account/dungeons, would you prefer it organized like :
v2/account/Dungeons.java
v2 (package)
|__ account (package)
|__ Dungeons.java
OR
v2/account/dungeons/Dungeons.java
v2 (package)
|__ account (package)
|__ dungeons (package)
|__ Dungeons.java

Tell me what you think, and I'll make changes for the Synchronous requests :)

@xhsun xhsun self-requested a review March 1, 2019 01:08
@xhsun xhsun added this to the v2.0.0 milestone Mar 1, 2019
@xhsun
Copy link
Owner

xhsun commented Mar 2, 2019

Sorry, I wasn't clear when I write the issue description for #43. So what I meant by split the request by the API endpoints is actually to put every path segment, in bold text, and its child segments (See here) in one request class.

For example, /v2/achievements have following child segments:

  • /v2/achievements/categories
  • /v2/achievements/daily
    • /v2/achievements/daily/tomorrow
  • /v2/achievements/groups

So the AchievementsRequest class will have the following methods for sync and async calls to /v2/achievements and its child segments:

  • getAchievementIds()

  • getAchievementIdsAsync(Callback<List<Integer>> callback)

  • getAchievements(int[] ids)

  • getAchievementsAsync(int[] ids, Callback<List<Achievement>> callback)

  • getAchievementCategoryIds()

  • getAchievementCategoryIdsAsync(Callback<List<Integer>> callback)

  • getAchievementCategories(int[] ids)

  • getAchievementCategoriesAsync(int[] ids, Callback<List<AchievementCategory>> callback)

  • getCurrentDailyAchievements()

  • getCurrentDailyAchievementsAsync(Callback<DailyAchievement> callback)

  • getNextDailyAchievements()

  • getNextDailyAchievementsAsync(Callback<DailyAchievement> callback)

  • getAchievementGroupIds()

  • getAchievementGroupIdsAsync(Callback<List<String>> callback)

  • getAchievementGroups(String[] ids)

  • getAchievementGroupsAsync(String[] ids, Callback<List<AchievementGroup>> callback)

I do realize with this way of splitting the requests, some request class will be really long (e.g., the request class for /v2/account and its child segments). But I think I'm more okay with it than what we have right now (i.e., everything in one class).

I see what you are trying to do and my apologies for not been clearer. But as you might realize, with your current approach we will end up with a literal tons of classes, which might cause us to go from one extreme (everything in one class) to an other extreme. However, I'm actually warming up to your current approach, I just not sure if I'm ready for the literal tons of classes.

I'm definitely open to suggestions. Thank you so much for what you have done. I really appreciate it!

@xhsun
Copy link
Owner

xhsun commented Mar 2, 2019

To answer your question, I prefer your first example:

v2/account/Dungeons.java
v2 (package)
| -- account (package)
      | -- Dungeons.java

@MiniClem
Copy link
Author

MiniClem commented Mar 2, 2019

Yes it's becoming really huge with 1 or 2 methods per class, I can see what you want, I'll finish the work with 1 endpoint and its children in one class
I see it better than my solution when the time to unit test it all will come, we would need the same context for many classes, so many code repeating itself in each children ^^

@MiniClem
Copy link
Author

MiniClem commented Mar 2, 2019

Ok now it should be better, gonna do sync requests now :)

@MiniClem
Copy link
Author

MiniClem commented Mar 2, 2019

Sync requests got the same name as async, what would be your way of choice to handle this ?
Split async and sync in different classes ? change method name ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants