Open dnlbauer opened 8 years ago
Here are a few concerns:
How would that work when using listeners to wait for the result?
Example:
// Asynchronously get summoner information
AsyncRequest requestSummoner = apiAsync.getSummonersById(region, summonerId);
requestSummoner.addListener(new RequestAdapter() {
@Override
public void onRequestSucceeded(AsyncRequest<Map<String, Summoner>> request) { // error because this is different from the overridden super method
Map<String, Summoner> summoners = request.getDto();
// ...
}
});
Sure, you could make RequestListener
a RequestListener<T>
, but then it could only listen to one specific method. Also nothing guarantees RequestListener<T>
to have the same T
as AsyncRequest<T>
, which makes things very fishy.
Another thought, what's with the design pattern to have one listener listen to every asynchronous request via RiotApiAsync#addListener()
? I have one use-case in production which actually implements RequestListener
and listens to all kinds of requests.
You could end up using a RequestListener<?>
here, but that's even more fishy, since then you will have zero knowledge about the actual return type.
Also changing the RiotApiAsync
methods this way would cause lots of conversion warnings which is dirty as hell:
public AsyncRequest<Map<String, List<League>>> getLeagueEntryBySummoners(Region region, long... summonerIds) {
Objects.requireNonNull(region);
Objects.requireNonNull(summonerIds);
return getLeagueEntryBySummoners(region, Convert.longToString(summonerIds)); // Type safety: The expression of type AsyncRequest needs unchecked conversion to conform to AsyncRequest<Map<String,List<League>>>
}
Yes, This would imply to change RequestListener
to RequestListener<T>
.
Also nothing guarantees RequestListener
to have the same T as AsyncRequest , which makes things very fishy.
I dont think so. addListener(RequestListener>
can be changed to addListener(RequestListener<T>)
. The addListener would only accept Listeners that can listen T then.
Also changing the RiotApiAsync methods this way would cause lots of conversion warnings
This is because type safty is not built in the other functions. If you start being typesafe from lowlevel on, they will disappear:
class EndPointmanager {
public <T> AsyncRequest<T> callMethodAsynchronously(method) { .....}
}
ApiMethod<Map<String, Summoner> method = new GetSummonersByName(getConfig(), region, Convert.joinString(",", summonerNames));
return endpointManager.callMethodAsynchronously(method);
I know its a big change and would require a lot of code being changed but I think its worth it. The global listener is a problem though. I have to think about how this could be solved. hm..
I recently had the idea to move towards a Future
This comment is mostly a reminder to myself to get back to it one day, but if anyone got any thoughts on this, please feel free to discuss and share your thoughts.
The current implementation of
AsyncRequest#getDto()
takes a generic parameter to specify the return type of the method. This makes it impossible to see whatasyncRequest.getDto()
is actually returning. It could be a summoner, a CurrentGameInfo or anything else. We only know it from looking at the context, but not from the object itself.Additionally it can lead to runtime exceptions like this:
I wonder what led to this design decision. Wouldnt it be more convenient to have a fixed return type with an implementation like this?
Best Regards, Daniel