3
\$\begingroup\$

I wrote this code to check an internet connection and was wondering if this is the best way to do this check.

public static boolean isNetworkAvailable(Context context) {
    boolean status = false;
    try {
        ConnectivityManager cm = (ConnectivityManager) context
                .getSystemService(Context.CONNECTIVITY_SERVICE);
        NetworkInfo netInfo = cm.getNetworkInfo(0);

        if (netInfo != null
                && netInfo.getState() == NetworkInfo.State.CONNECTED) {
            status = true;
        } else {
            netInfo = cm.getNetworkInfo(1);
            if (netInfo != null
                    && netInfo.getState() == NetworkInfo.State.CONNECTED)
                status = true;
        }
    } catch (Exception e) {
        e.printStackTrace();
        return false;
    }
    return status;
}
\$\endgroup\$
  • \$\begingroup\$ What is the best approach depends a lot on what you need to use it for. I am guessing you are writing an application, which will communicate with one specific service. And you want to know if you are online in order to switch between an online and an offline mode. In that case you are better off simply trying to communicate with that service and if you get an error or if you haven't started exchanging data within a few 100 ms, then assume you are offline. \$\endgroup\$ – kasperd Jun 27 '15 at 17:51
  • \$\begingroup\$ If you are not aiming to communicate with one specific service and you just want to know what the UI is currently showing about the online status to the user, you should be aware that this is not as simple as just online or offline. For WiFi the status bar can either indicate offline, connected to an access point, or connected and able to communicate with services on the internet. I believe for cellular connectivity there is even more variation in the possible states. \$\endgroup\$ – kasperd Jun 27 '15 at 17:54
7
\$\begingroup\$

I am not familiar enough with Java to know what inside your try block could throw an exception or what sort of exceptions you might expect, however, simply printing the stack trace surely can't really be all that helpful.

First of all, any end user isn't going to see this information. Second of all, no developer will see this testing outside of the development environment.

Again, I'm not that familiar with Java, so I don't know what options are available, but you need to do one of a few things.

  1. Show the exception to the user (or some information about it anyway). This is probably completely unnecessary. The network connection is either available or not, right?
  2. Rethrow the exception and let the caller handle it. It can be useful to know there was an exception and we should retry the network connection later.
  3. Log the exception in a way that the developer can access even when it happens in production. Error logs should be uploaded somewhere.
  4. Just do nothing, and don't waste execution time printing the stack trace. If you chose this option, you probably want to find a way to leave the print stack trace in for development environments, but for production builds, you shouldn't be executing this code.

Now.... our code also has some magic numbers in it. I realize this isn't entirely your fault. I've visited the official documentation. I see, that somehow, this method simply takes an int and the documentation doesn't seem to clarify what the legitimate values are very well... but they are all defined as constants:

ConnectivityManager.TYPE_BLUETOOTH
ConnectivityManager.TYPE_DUMMY
ConnectivityManager.TYPE_ETHERNET
ConnectivityManager.TYPE_MOBILE
ConnectivityManager.TYPE_MOBILE_DUN
ConnectivityManager.TYPE_MOBILE_HIPRI
ConnectivityManager.TYPE_MOBILE_MMS
ConnectivityManager.TYPE_MOBILE_SUPL
ConnectivityManager.TYPE_VPN
ConnectivityManager.TYPE_WIFI
ConnectivityManager.TYPE_WIMAX

These are the constants we should be passing in when checking for a connection. Using these constants makes things a bit more readable. But moreover, I might modify our method to be more flexible. You're only checking for WIFI or MOBILE (I don't know enough about Android to know if this is excluding anything important). But we can write our function in such a way that we can check for whatever types the user needs.

public static boolean isNetworkAvailable(Context context, int[] networkTypes) {
    try {
        ConnectivityManager cm = (ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE);
        for (int networkType : networkTypes) {
            NetworkInfo netInfo = cm.getNetworkInfo(networkType);
            if (netInfo != null && netInfo.getState() == NetworkInfo.State.CONNECTED) {
                return true;
            }
        }
    } catch (Exception e) {
        return false;
    }
    return false;
}

Now our method is duplicating less code and its flexible enough to check for what ever types of networks the user happens to specify. If we want a method that doesn't require passing in networks and still does what your original implementation does, we can just write a wrapper method that passes mobile/wifi in to this method.

\$\endgroup\$
  • \$\begingroup\$ this completely right answer , but can I check with getActiveNetworkInfo() , better thank make multi check , what do you think ? \$\endgroup\$ – Mina Fawzy Jun 27 '15 at 13:57
  • \$\begingroup\$ I don't know enough about Java specifics, but I know that sometimes you'll definitely only want to check if you have a WiFi connection. And I'm sure there are times when you will want to check for other specific connection types, not just any active connection. \$\endgroup\$ – nhgrif Jun 27 '15 at 14:01
  • \$\begingroup\$ yea this right , you save my day but this nice code , I will make you review all my code lol \$\endgroup\$ – Mina Fawzy Jun 27 '15 at 14:06

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