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?

Dialogize

No, in object orientated programming if you have a class ending in Helper you have an ill defined object it is known as a code smell – Boudoir

Lots of good arguments here. Even if you stick with what you currently have, wouldn't it be better to hold WeakReferences of perishable goods such as Activity, Context etc.? – Silin

Answer 1

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.

Messieurs

Answer 2

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 :)

Cannady

This presentation does not really answers my question here. – Dialogize

Yes it does because it shows the pattern you should be using in your case – Mooneye

I agree with Peter M. This is the right answer. It's even more. This presentation show you exactly how you should organise and structure network operations. I have implemented this(option A 15:00min) in my app and it works really well. Regarding helper, it should provide separate methods for different kind of calls and just start Service responsible for the rest. Additionally, it should check if requested process is already working. It's a great Google presentation. – Macintyre

Answer 3

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.

Courtund

Answer 4

It's always a good practice to make code more readable and easier to maintain. Encapsulating responsabilities is one of the best ways of doing both.

And a good argument to your boss would be: The connection between your View and Server STARTS at the View and END in the server, but the one who get it done (and is RESPONSABLE for it) is this class. Doesn't matter to the Activity or the Server how it's done, only the outcome.

---

Some modifications based on the code you've shared that i would suggest:

• Remove the constructor and getInstance()
• Create a initiator() method and initiate AsyncHttpClient, Gson and other params
• Create other classes that extends your helper and implement their own especific performNetworkTask() (they could and shoud have their specific method name as well)
• UseperformNetworkTask() statically
• Start handling callbacks (the View is responsable for this)

Would be something like this:

public class HttpsClientAsyncHelper {
   Acitivity activity;
   AsyncHttpClient client;
   Gson gson;

   // Other params

   public static void initiator(Context context)
   {
       activity = (Activity) context;
       client = new AsyncHttpClient().setCookieStore(CookieUtil.getInstance(activity)
                                .getPersistentCookieStore());
       gson = new GsonBuilder().setDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'")
                               .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
                               .create();
   }

   // Other methods
}

And create 10 more classes like this :

public class RemoveCreditCardAsyncTask extends HttpsClientAsyncHelper {

   public static void performNetworkTask(Context context)
   {
       initiator();
       // Do what you did in this specific case
   }
}

Usage:

 RemoveCreditCardAsyncTask.performNetworkTask(this);
 // specific method names
 AutoRenewAsyncTask.enableAutoRenew(this);
 AutoRenewAsyncTask.disableAutoRenew(this);
 // even more specific
 AutoRenewAsyncTask.enable(this);
 AutoRenewAsyncTask.disable(this);

---

I practice this myself in an even more generic class called Network that handles all the communication with my WebService:

serverPostRequest(String controller, String action, JSONObject jsonParameters) throws Exception {}

fromJson(String json, Class<T> classOfT) throws JsonSyntaxException {} 

Bonus: A probably usefull method for you:

public static boolean isAvailable(Context context)
{
    ConnectivityManager connectivityManager = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
    NetworkInfo activeNetworkInfo = connectivityManager.getActiveNetworkInfo();

    return activeNetworkInfo != null && activeNetworkInfo.isConnected();
}

Arand

This solution further complicates the case, and really hard to modify. How would you add request parameters to the extended classes performNetworkTask? Why initiator? That is what constructor made for... or static block if you really want to access your methods in a static way. Not to mention it is easy to forget about calling initiator, and that way the task would fail. – Messieurs

Adding those parameters to the method params? initiator() is just a way to replace the constructor in a static way (could be a static block as well). He doesn't need to use an initiator method, but he needs to initiate variables before using them. I was trying prove that his point of view is correct and used a quick example to show the concept. This is the OP question: So who is right? is it really a bad practice to create a class like this? – Arand

How would you add parameters to the requests? Like, what if a request needs two parameter (for example bankCardNum, and userId)? In your example the initiator method has to be called, otherwise the task would fail, I just wanted to point that out. Yes I know what is his question, and it is a bad practise, at least in my opinion. – Messieurs

like this performNetworkTask(Context context, JSONObject jsonParameters)? Why does the initiator needs to be called? It's just a handy method. He could just instantiate his own parameters there before using them (like Activity activity = (Activity) context). I thank you for trying to add info to the answer. Feel free to edit it anytime. I just really think it's obvious how to add parameters and that parameter initiation is required. I liked the static block one though. – Arand

You cannot add paramters that way:) Owerridden methods must have the same signature. That would define an other function, so the derived class would have 2 performNetworkTask. Which one to call? Which one to implement? ... Allright so tell me, what is the reason of extending from HttpsClientAsyncHelper if you have to initialize/implement everything again? – Messieurs

NOW i see your point, but it's not overridden hahah. And the idea is to have other usefull methods in the parent (like some the OP had posted in the original question or in my example isAvailable(), fromJson() etc). There will be probably lots of other methods, it will be parent of all connections to his server – Arand

Answer 5

Perhaps this link from Misko Hevery:

http://misko.hevery.com/2008/08/17/singletons-are-pathological-liars/

and its follow-up:

http://misko.hevery.com/2008/08/21/where-have-all-the-singletons-gone/

will help. He specifically deals with singletons in a credit card app and it has implicit MVC. MVC is ubiquitous. You usually need a good reason to depart from it.

You also have a large switch() in your logic. Usually that is handled by Polymorphism in OO. Methods are good at encapsulating functionality in a unit-test-friendly way.

Extortioner

Answer 6

I totally agree with your boss about that he said, it should be a dumb class. Instead of a Helper class you most likely need an Executor or Worker kind of a class. Singletons are most of time evil after all also you will leak your context eventually and end up memory leaks.

There is a way better approach, use self retained Fragments.

Now consider this, turn your helper class into a self retained Fragment which should be the connection holder for your system and should be able to execute some tasks over that connection. Also lose that switch...case and turn your tasks some kind of Runnable or Callable like classes.

Now you have a Fragment based executor like class and a bunch of task that can execute. This approach will bring you this advantages:

• Since our fragment is self retained, it will act like a singleton,
• You can use this fragment between activities, which is something you can't do when using a singleton,
• Since fragment itself is a android component, android will provide you a context whenever you need it so you will not have to worry about leaking context or memory.
• Even if you lose the current context(lets say user used home button) when some of your task is running, you can simple wait till another context is available(fragment re-attached to some activity)

By the way you can also use a Service for this instead of Fragments, same principles but might be a little bit harder to implement because Service-Activity communications adds another layer of complication.

Comeuppance

Answer 7

Try this approach for your network related task:

1. Create HttpHelper class in which initialize http object and write helper methods to execute http requests.

2. Create separate AsyncTask for all your http requests and from this class initialize HttpHelper and perform network operations which gives 2 benefits:

a) Your code remains readable and modular which is easier to maintain and debug.

b) As the network requests are independant, it gives freedom to utilize this class anywhere within the application.

3. Communicating back from AsyncTask Interface, Handler can be used or you can directly pass the activity object but this way you can leak the context which can lead to memory leak.

Streetwalker

Answer 8

Answer the following questions:

1. Will you need to repeat some logic in each of the methods of the class? If yes - then most probably you need to split it into more classes, in order to prevent code duplication.
2. Will your class have both customer-facing output (View), internal business logic (Controller) and data access/persistence logic (Model) - if it would need to have any both of these, then you would probably like to split it in order to separate presentation from business logic and data access (MVC).
3. Does your class contain logic that could be potentially useful in other use cases/integrations/etc.? If yes, you might want to make this logic into a separate class (which to inherit or include as a property in your actual class.

Interlunar