patternjavaMinor
AsyncTask for handling server api calls
Viewed 0 times
callshandlingforserverapiasynctask
Problem
Got this design for getting rid of checking network availability and informing user of errors. Is it good at all? Please, help.
Overall point is to fire server call as short as possible:
```
abstract class AbstractServerCallTask extends AsyncTask>{
@Override
protected AsyncResult doInBackground(Void... arg0) {
if (!AppUtils.isNetworkAvailable())
return new AsyncResult("No network available. Please, check your connection");
try {
return new AsyncResult(callServerMethod());
} catch (ConnectionException e) {
return new AsyncResult("Can't connect to server, chek your network state");
} catch (InternalServerException e) {
return new AsyncResult("Server-side fault. Please, try again later");
} catch (JSONException e) {
return new AsyncResult("Wrong data format. Please try again later");
}
}
abstract Result callServerMethod() throws ConnectionException, InternalServerException, JSONException ;
@Overr
Overall point is to fire server call as short as possible:
new DefaultServerCallTask(){
@Override
protected void onSuccess(LoginResponse response){
//go to nextActivity}
}
@Override void callServer() throws ConnectionException, InternalServerException, JSONException {
return ServerApi.login(/*enclosed params from outer variables */);
}
}.execute();AsyncResult is a wrapper class, it's main purpose to be returned from AsyncTask.doInBackground.CheckableResult is an inteface for POJOs, returned from server.class AsyncResult{
T mResponse;
boolean mOk;
String mErrorMessage;
T getResponse(){
return mResponse;
}
boolean isOk(){
return mOk;
}
String getErrorMessage(){
return mErrorMessage;
}
public AsyncResult (T response){
mErrorMessage = "";
mOk = true;
mResponse = response;
}
public AsyncResult (String errorMessage){
mErrorMessage = errorMessage;
mOk = false;
}
}AbstractServerCallTask handles connection and server-side exceptions^```
abstract class AbstractServerCallTask extends AsyncTask>{
@Override
protected AsyncResult doInBackground(Void... arg0) {
if (!AppUtils.isNetworkAvailable())
return new AsyncResult("No network available. Please, check your connection");
try {
return new AsyncResult(callServerMethod());
} catch (ConnectionException e) {
return new AsyncResult("Can't connect to server, chek your network state");
} catch (InternalServerException e) {
return new AsyncResult("Server-side fault. Please, try again later");
} catch (JSONException e) {
return new AsyncResult("Wrong data format. Please try again later");
}
}
abstract Result callServerMethod() throws ConnectionException, InternalServerException, JSONException ;
@Overr
Solution
There are many things that you do well here. Overall the code is fine.
I have some points that you should seriously consider and some things that you should think about. In the end, this is your code and I don't know everything that you know about it's purpose, usage and further plans.
Seriously consider
-
Android provides a lovely way to handle String Resources. I suggest you make use of that. Not only is it very handy to keep all your strings stored in
-
As
Think about
-
What if the server provides both an error message and more detailed information as a response? I suggest that you create a constructor for your
-
I find the naming of
-
Your
I think that you should consider allowing change of the first
-
It is OK to have a
I want to leave you with something positive though: Overall, your code is fine. So for your question "Is it good at all?" I have to say "yes".
I have some points that you should seriously consider and some things that you should think about. In the end, this is your code and I don't know everything that you know about it's purpose, usage and further plans.
Seriously consider
-
Android provides a lovely way to handle String Resources. I suggest you make use of that. Not only is it very handy to keep all your strings stored in
.xml-files but it is also impossible to internationalize your messages if you hardcode string values within your code.-
As
AsyncResult seems to be a good, flexible, generic (generic as in both Java-generics and English generic) utility class, it wouldn't hurt to put getters as public and the fields as private finalThink about
-
What if the server provides both an error message and more detailed information as a response? I suggest that you create a constructor for your
AsyncResult which accepts both T response and String errorMessage. You can then use constructor chaining in your class. Constructor chaining is a good way to reduce code duplication in constructors.-
I find the naming of
DefaultServerCallTask to be a bit misleading, the usage of the word Default together with the keyword abstract on the same class doesn't make sense to me. If it really is Default, then why is it abstract? (Although I understand your reasons for having it this way, I question the naming of the class).-
Your
AbstractServerCallTask extends AsyncTask>. It is functional and I understand that you can specify data to be sent to the server by calling a constructor and storing it in a field, or creating an inner anonymous class which you actually do when you call new DefaultServerCallTask(){ ... }.execute();.I think that you should consider allowing change of the first
Void generics, which is the Param class. If you would allow changing that, then you can pass arguments to your execute method, which can be passed on to callServerMethod instead of having to get variables from outside into your new DefaultServerCallTask(){...-
It is OK to have a
AsyncResult class, but will you have any use for the generics in that class outside your server calls? Your server calls only makes use of AsyncResult. As hinted by rolfl in his comment, a user currently can't use a AsyncResult along with your classes for server calls.I want to leave you with something positive though: Overall, your code is fine. So for your question "Is it good at all?" I have to say "yes".
Context
StackExchange Code Review Q#36298, answer score: 5
Revisions (0)
No revisions yet.