Is it good practice to develop a helper network class that is responsible for all network tasks?

Question

I have created the following class:

public class AsyncHttpsClientHelper {

public static final int REMOVE_CREDIT_CARD = 1;
public static final int ENABLE_AUTORENEW = 2;
// +10 final ints...

private static AsyncHttpsClientHelper instance = null;
private static Activity activity;
// Some other variables

    private AsyncHttpsClientHelper(Context context) {
        // Initiate variables
    }

    public static AsyncHttpsClientHelper getInstance(Context context) {
        // Guarantees the same instance for this class (Singleton)
    }

    public void performNetworkTask(int networkTaskType, String value)
    {
        switch (networkTaskType)
        {
            case REMOVE_CREDIT_CARD:
            {
                CupsLog.d(TAG, "path: " + Consts.ACCOUNT_REMOVE_CREDIT_CARD_PATH);
                client.post(Consts.ACCOUNT_REMOVE_CREDIT_CARD_PATH , new JsonHttpResponseHandler() {

                 @Override
                 public void onSuccess(JSONObject result) {
                try {
                    CupsLog.d(TAG, Consts.ACCOUNT_REMOVE_CREDIT_CARD_PATH + " -> onSuccess, result: " + result.toString(3));
                    AccountService.getInstance(activity).pullAccountDetailsFromServer();
                    Toast.makeText(activity, "Your credit card was removed", Toast.LENGTH_SHORT).show();
                } catch (JSONException e) {
                    e.printStackTrace();
                }
            }

                 @Override
                 public void onFailure(Throwable arg0) {
                    CupsLog.d(TAG, "commitPaymentToServer -> onFailure");
                     BusProvider.getInstance().post(new DialogChangeEvent(DialogChangeEvent.REMOVE_CREDIT_CARD, "failed"));
                }
        });
        break;
            }
            case ENABLE_AUTORENEW:
            {
                // Do ENABLE_AUTORENEW logic
            }
            // +10 cases goes here...
        }
    }
}

This class is not finished yet and I still have to add here another 10 other network calls I perform all around the application.

To run one of the network tasks I run this line for example:

AsyncHttpsClientHelper.getInstance(this).performNetworkTask(AsyncHttpsClientHelper.COMMIT_COUPON_TO_SERVER, event.getValue());

When the task finishes I fire an Event using the Square event Bus tool to commit the needed visual changes to the activity/ fragment layout.

The Question: My boss claims that this is a bad practice and that this class will become a mess when I finish it. More over he claims that this class should be a stupid class that all he knows how to do is to configure the AsyncHttpClient object and return it so I could use it and perform the https tasks inside the relevant Activity. Basically he says the the https calls themselves should be located in the Activity classes. I like this way better and think it makes my activities more cleaner and nicer to read. He on the other hand says that this way its harder to debug and that this class combines a part of Controller and View functionality which it should not do.

So who is right? is it really a bad practice to create a class like this?

Answer 1

Look at this presentation from Google IO 2010 about Android REST client apps -I think its complete answer for Your question.

https://www.youtube.com/watch?v=xHXn3Kg2IQE

Cheers :)

Answer 2

You keep static reference to your Activity - This is bad practice! What if the phone is rotated during long-running network operation.

The most robust and clean solution described in Google I/O presentation.

Your solution has some disadvantages:

• singleton is not thread safe
• it uses Activity context - you can easily get memory leak or call dead activity UI
• UI and network calls are tightly linked
• it supports only one Activity

As per your question - yes call AsyncHttpClient in place will result in more cleaner code that you suggests.

Answer 3

Consider what Dimitry said about rotation, because its true. Never use static reference to objects that have any connection to the UI. It causes memory leaks, and it is very hard to debug.

Other problems:

• Forget about getInstance(Context c). (actually, you had to write this because of the static context reference)
• Your performNetworkTask method has a "value" attribute that is only used in one of the tasks. Its confusing, noone would know what "value" is, only the person who wrote this code. What if you would need other parameters for other requests? Append them as parameters like "value"? It would be a mess. You should instead create an abstract Request class, and derive a new class for every request type. That way it would be much easy to understand what happens, and would be easy to extend the functionality also.
• I guess your boss is trying to say that you should not wire the callbacks into your helper class. The networking can be handled by a separate class (optimally by a Service, or by the async http client), but the callback should definately be in the Activity, since that is where the reaction happens. How would you react in different ways for a request from separate Activities? With your implementation you can not, because all the callbacks are wired. Take a look at the site of the library, it has a relatively good example on how to achieve this: http://loopj.com/android-async-http/#recommended-usage-make-a-static-http-client

And to answer your questions:

My boss claims that this is a bad practice, and that this class will become a mess when I finish it.

Yes, it will become a mess (and will become even worse as you start to add/modify functionalities).

More over he claims that this class should be a stupid class that all he knows how to do is to configure the AsyncHttpClient object and return it so I could use it and perform the https tasks inside the relevant Activity.

It should be stupid. In my opinion, you can keep the performNetworkTask method, BUT you need to have callback parameters to let callers react different.

public void performNetworkTask(Request request, JsonHttpResponseHandler handler);

You should also drop int networkTaskType. My suggestion is to use an abstract Request class. However if you prefer this way, then switching to Enum is a minimum.

Basically he says the the https calls themselves should be located in the Activity classes.

"Https calls" can mean anything. I think he meant the callbacks should be passed by the caller.

I like this way better and think it makes my activities more cleaner and nicer to read.

This way you definately have less code in your Activity, but you can support only one Activity! Basically, you just moved the missing code from the Activity to this helper class. Not to mention that it would be a pain to modify the helper in any way.

He on the other hand says that this way its harder to debug and that this class combines a part of Controller and View functionality which it should not do. So who is right? is it really a bad practice to create a class like this?

• I agree on that you try to wire the View part to a helper class.
• I don't know what he means about "hard to debug", so the best would
  be to ask him.
• And yes, it is a bad practise. You should refactor.