3
\$\begingroup\$

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:

new DefaultServerCallTask<LoginResponse>(){

    @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>{

    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<Result extends CheckableResponse> extends AsyncTask<Void, Void, AsyncResult<Result>>{


@Override
protected AsyncResult<Result> doInBackground(Void... arg0) {
    if (!AppUtils.isNetworkAvailable())
                return new AsyncResult("No network available. Please, check your connection");
    try {
        return new AsyncResult<Result>(callServerMethod());
    } catch (ConnectionException e) {
        return new AsyncResult<Result>("Can't connect to server, chek your network state");
    } catch (InternalServerException e) {
        return new AsyncResult<Result>("Server-side fault. Please, try again later");
    } catch (JSONException e) {
        return new AsyncResult<Result>("Wrong data format. Please try again later");
    }
}

abstract Result callServerMethod() throws ConnectionException, InternalServerException, JSONException ;

@Override
protected void onPostExecute(AsyncResult<Result> asyncResult){
    if (asyncResult.isOk()){
        Result r = asyncResult.getResponse();
        if (r.isOk())
            onSuccess(r);
        else
            onRequestFailed(r); // we got to server, but somehing is wrong
    }
    else
        onError(asyncResult.getErrorMessage());
}
protected abstract void onError(String errorMessage);
protected abstract void onRequestFailed(Result r) ;
protected abstract void onSuccess(Result r);
}

DefaultServerCallTask leaves onSuccess for derived innerclasses to override:

abstract class DefaultServerCallTask<Result extends CheckableResponse> extends AbstractServerCallTask<Result>{


@Override
protected void onError(String errorMessage) {
    AppUtils.showToast(errorMessage);

}

@Override
protected void onRequestFailed(Result r) {
    AppUtils.showToast(r.getErrorMessage()); // show server-side generated message
}
}
\$\endgroup\$
  • \$\begingroup\$ What if someone wants to get a String response from the server, can he create an AsyncResult<String>? \$\endgroup\$ – rolfl Nov 28 '13 at 15:57
  • \$\begingroup\$ It is not meant to be used that way. You'll have to make a wrapper for String, implementing CheckableResult (isOk, getError etc, very AsyncResult alike). \$\endgroup\$ – Eugene Nov 28 '13 at 18:32
5
\$\begingroup\$

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 .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 final

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 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<Void, Void, AsyncResult<Result>>. 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<LoginResponse>(){ ... }.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<LoginResponse>(){...

  • It is OK to have a AsyncResult<T> 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<Result>. As hinted by rolfl in his comment, a user currently can't use a AsyncResult<String> 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".

\$\endgroup\$
  • 1
    \$\begingroup\$ Simon, thank you very much for your detailed answer, i really appreciate it. Actually asyncresult isn't meant to be used outside of abstractservercalltask, it is needed for moving exceptions handling to main thread only. It is my inconvenience that have left it outside abstractservercalltask. About resource strings - i haven.t posted this part for the sake of simplicity, and I absolutely will incorporate in in final code. I should refine code now and think of other points you've made. \$\endgroup\$ – Eugene Nov 28 '13 at 18:23
  • \$\begingroup\$ Awesome explaination. It will help all new devs to understand the design pattern and generics. Kudos @Simon for detailing. \$\endgroup\$ – Sachin Shelke Apr 25 '16 at 2:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.