About memory leaks and generic methods

Question

Which of these is better to use in my app?

public class NetworkCheck {

    Context context;

    public NetworkCheck(Context context) {
        this.context=context;
    }

    public boolean isNetworkConnected() {
        ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
        return cm.getActiveNetworkInfo() != null;
    }
}

...    

if(new NetworkCheck(this).isNetworkConnected()){
    //statement
}

For the above one I have to create heap memory every time whenever I have to use its method. Its heap memory will be destroyed when its scope ends (means end of curly braces)...

Alternatively.

public class NetworkCheck {

    public static boolean isNetworkConnected(Context context) {
        ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
        return cm.getActiveNetworkInfo() != null;
    }
}

...

if(NetworkCheck.isNetworkConnected(){
    //statement
}

For this one I don't have to create any heap memory. I read many articles where people are saying creating a static variable and method causes a memory leak in the application.

and please help me to c create this Genric getLocalData() of the below method .....

 public static <T> void saveLocalData(Context context, String key, T value) {
        SharedPreferences prefs = context.getSharedPreferences(
                "Qikqrup", Context.MODE_PRIVATE);
        SharedPreferences.Editor editor = prefs.edit();
        if (value instanceof String)
            editor.putString(key, (String) value);
        else if (value instanceof Boolean)
            editor.putBoolean(key, (Boolean) value);
        else if (value instanceof Integer)
            editor.putInt(key, (Integer) value);
        editor.commit();
    }

Answer 1

saveLocalData() method will not cause any memory leak as you are not holding back any reference longer than its lifetime.

You should take care of below point while using activity context :

You should not use activity context in class which can outlive activity's lifecycle.

For e.g : don't use activity context in database helper class or singletons.

Regarding NetworkCheck, class2 is better as isNetworkConnected() method is a utility method and it doesnt represent any state of NetworkCheck.

Pros:

1. improves performance since you're not instantiating an extra object.

2. not dependent on instance creation

3. can be easily used throughout your app(in different classes where you are performing network operation)
4. will make code easier to understand

Cons :

1. Can't be easily mocked for unit testing.

Answer 2

The second case cannot be beaten, as it is tightest.

In the first case as long as the NetworkCheck object lives, the Context object will be kept alive. And there is a second object (NetworkCheck).

However:

• its use is limited and the object will rapidly go out of scope and be garbage collected. So the effect is as remarkable as the spontaneous creation of particle and antiparticle around us.
• In other cases the separation of constructor and call, maybe many calls might prove usefull.

About:

public static <T> void saveLocalData(Context context, String key, T value)

Not usefull, Object would be better. In fact null will not be catched. In general this function has a partial domain: not all classes will do. And again retrieving the value will be guess work.

Using run-time information might do.

public static <T> void saveLocalData(Context context, String key, Class<T> type, T value)
public static <T> T loadLocalData(Context context, String key, Class<T> type)

Java has a Serializable interface that can be used too.

public static <T> void saveLocalData(Context context, String key, Serializable value)

Then you receive the object back as you stored it. You will need to read up on the subject.

Answer 3

In your saveLocalData() method, there isn't necessarily a memory leak, but there can be a memory leak, depending on how you use it. For example, take this:

for( long i = 0;  i < Long.MAX_VALUE;  i++ )
{
    String name = String.valueOf( i );
    saveLocalData( context, name, i );
}

This loop will keep adding values to your map until you run out of memory. But the same can happen with any collection, not just with maps, and it can happen whether your collection is statically allocated or not. With a static collection it is just slightly easier to happen than with a non-static collection, because presumably the non-static collection could conceivably go out of scope and be garbage collected, while the static collection is usually destined to stay forever.

However, there is nothing absolute about this statement:

• On one hand, a non-static collection may very easily be permanently anchored to memory by being referenced by an object which is static, while:

• On the other hand, a static map may be explicitly freed and reallocated or simply cleared by a careful programmer who does not want it to grow too big.

---

In order to write getLocalData() you need to declare it as follows:

public static <T> T getLocalData( Context context, String key, Class<T> classOfValue )

And invoke it as follows:

String name = MyClass.getLocalData( context, "Name", String.class );

within the function, you are going to need to do something like this:

if( classOfValue == String.class )
    return editor.getString( key );
...

---

Your two versions of NetworkCheck and your intended usage of each is roughly equivalent to each other, there is not much to gain or loose between choosing one or the other. Doing new NetworkCheck(this) represents a redundant memory allocation, but as you already understand, this memory gets garbage-collected very quickly, so there is no damage.

Answer 4

public class AppStatus {

private static AppStatus instance = new AppStatus();
static Context context;
ConnectivityManager connectivityManager;
boolean connected = false;

public static AppStatus getInstance(Context ctx) {
    if (ctx != null)
        context = ctx.getApplicationContext();
    return instance;
}

public boolean isOnline() {
    try {
        connectivityManager = (ConnectivityManager) context
                .getSystemService(Context.CONNECTIVITY_SERVICE);

        NetworkInfo networkInfo = connectivityManager.getActiveNetworkInfo();
        connected = networkInfo != null && networkInfo.isAvailable() &&
                networkInfo.isConnected();
        return connected;


    } catch (Exception e) {
        System.out.println("CheckConnectivity Exception: " + e.getMessage());
        Log.v("connectivity", e.toString());
    }
    return connected;
 }
}

And to check if application status is online or not, check via

if ("package Name".AppStatus.getInstance(getActivity()).isOnline()) {}

Answer 5

I would recommend using the static method version here in this case since it's excessive to create a new object just to check connectivity.

"Static methods are just methods, they are not stored on the heap, they just don't get to use a "this" parameter."

About memory leaks and generic methods

Answer 6

STATIC variable and method causes the MEMORY LEAK

This is true only for static fields of a class, not the methods as well. And this is true because the values of static members will live in memory for as long as the class declaring them will reside in memory - (or, at least, as long as you don't set the static vars value to null allowing the GC to recycle the memory).
(btw: the correct terminology is "instances stored cause long term memory consumption" It only becomes a leak if those instances are never used, otherwise they become "useful long term memory use").

As for static methods - if they don't store in static variables any instances they are creating, then those instances will either:

1. if local vars/non-return instances - be CG-ed asap after going out-of-scope (note: "asap" doesn't guarantee "immediately" - this is what "as possible" means after all)

2. if they are return value - they'll be GC-ed only after the caller release them (i.e. no longer "store" them for future uses). In respect with the returned values, your static method acts like a 'factory': ignoring the specific sequence of configuration (assumed to be more complex than a constructor can provide), they are no different from an instance create with just new ClassOfTheReturnedValue(...)

In regards with public boolean isNetworkConnected() versus static public boolean isNetworkConnected(): in your example of use, there's no difference between them in respect with memory-leaking or the ability to create instances that will be GC-ed - because your NetworkCheck check=new NetworkCheck(this); is just a temporary/local variable which will be collectable once it goes out of scope.
The only difference between the two approaches: the non-static really creates an object (thus more work for CG down the track), while the static form doesn't.

---

Regarding public static <T> void saveLocalData(Context context, String key, T value)

Well, the way you posted it is grossly suboptiomal.

1. if calling that method with anything except String, Boolean, Integer, Long or Float the compiler will wire the call as requested. At execution time, you'll spend unnecessary type checks (all failing), only to call editor.commit() with no real change

2. In regards with type-safety, you deny the compiler any possibility of perform the type checking at compile time to go with an all runtime type-check. Your method is no different to public static void saveLocalData(Context context, String key, Object value) - all the type checking are deferred to runtime. The only difference between the generic and 'plain-object' forms is: the compiler will work harder to compile the generic method, but will reach the same final result as for the 'Object value' method.

The best way to achieve the same is to drop all the smartness of generics and use the plain-old-method-overload, like this:

protected static SharedPreferences.Editor resolveEditor(
  Context context, String prefOwner
) {
    SharedPreferences prefs = context.getSharedPreferences(
            prefOwner, Context.MODE_PRIVATE
    );
    return prefs.edit();
}
public static void saveLocalData(Context context, String key, boolean value) {
    SharedPreferences.Editor editor = resolveEditor(context, "sushildlh");
    editor.putBoolean(key, value);
    editor.commit();
}
public static void saveLocalData(Context context, String key, int value) {
    SharedPreferences.Editor editor = resolveEditor(context, "sushildlh");
    editor.putInt(key, value);
    editor.commit();
}
public static void saveLocalData(Context context, String key, String value) {
    SharedPreferences.Editor editor = resolveEditor(context, "sushildlh");
    editor.putString(key, value);
    editor.commit();
}

// etc

With the above:

1. the compiler can verify that the method is not called with values of types that don't make sense

2. the code is optimal

After all, the SharedPreferences.Editor does have a reason for not implementing a generified void putValue<T>(T value) - what makes you think you can do better from outside of it? If that would be possible, I reckon the authors of SharedPreferences.Editor would have done it inside their implementation.