1
\$\begingroup\$

I'm new at Android and Java development and I've put together a demo app to start learning, which is made of:

  • a main activity extending ActionBarActivity, in which
  • a ViewPager is instantiated which has
  • a FragmentPagerAdapter responsible for showing up...
  • one Fragment out of three at a given time which
  • when it's created, instantiates a AsyncTask subclass (defined in a separate class) that when done (onPostExecute)...
  • populates a TableLayout in the Fragment that just fired it.

I'm also trying to keep compatibility with older Android platforms, so I'm using support library where necessary.

That said, I'd kindly appreciate if I could:

  1. get a general code review, plus some advices on a couple of points
  2. get some advices with regards to how memory is allocated/deallocated when an application is correctly running

1: Code review and advices

  • From an all around perspective, is this code ok, expecially with regards to avoiding any memory leaks ?

  • I am unsure about having to pass both the view and the context to the AsyncTask object. Could I just pass one of those and infer the other someway?

  • In the onPostExectute method I am trying to build a table layout where each row has three TextViews (3 columns) and it feels a bit as if I am forcing that idea in that type of layout. I mean, am I using the right layout for this layout idea? Any other nicer way to do this?

2: Advice on behaviour of memory allocation/deallocation

Is what you see from the memory profiling screenshot the expected good behaviour for a non-leaking app? I mean, am I supposed to see memory being filled up, even when there is no leaking, (provided at some point it gets garbage collected)?

Android Studio memory profiler

I don't know how to better put this, but am I expected to see this kind of graph when everything is going ok? Also, as you can see, the first time, the GC is only invoked when memory is completely filled up, but afterwards GC triggers in sooner, when there is still some memory available (I am still stressing the app though). Is this normal?

Thanks for your help.


Please note: this app was experiencing some kind of weird bug with AsyncTask, where HTTP requests that are fired in AsyncTask got occasionally stuck and errored out with connection refused and connection timeout exceptions, expecially after first run of garbage collector. Now that ended up fixing itself (laughter allowed) after a reboot (I am left with thinking that it was due to some glitches in Android Studio) and the application is now fully working as expected but given I spent a lot of time trying to debug that behaviour, I created a question just for that on StackOverflow, removing that part of the question from here as adviced in comments.


MainActivity.java

package com.mydom.demoapp;

import android.os.Bundle;
import android.support.v4.app.FragmentTransaction;
import android.support.v4.app.NavUtils;
import android.support.v4.view.ViewPager;
import android.support.v7.app.ActionBar;
import android.support.v7.app.ActionBarActivity;
import android.util.Log;
import android.view.Menu;
import android.view.MenuItem;

import com.mydom.demoapp.adapter.TabsPagerAdapter;

public class MainActivity extends ActionBarActivity implements ActionBar.TabListener {

    private ViewPager viewPager;
    private TabsPagerAdapter mAdapter;
    private ActionBar actionBar;

    private String[] tabs = { "Music", "Movies", "News"};

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        setContentView(R.layout.activity_main);

        viewPager = (ViewPager) findViewById(R.id.pager);
        mAdapter = new TabsPagerAdapter(getSupportFragmentManager());
        viewPager.setAdapter(mAdapter);

        actionBar = getSupportActionBar();
        actionBar.setDisplayHomeAsUpEnabled(false);
        actionBar.setHomeButtonEnabled(true);
        actionBar.setNavigationMode(ActionBar.NAVIGATION_MODE_TABS);

        // Adding Tabs
        for (String tab_name : tabs) {
            actionBar.addTab(actionBar.newTab().setText(tab_name).setTabListener(this));
        }

        // on swiping the viewpager make respective tab selected
        viewPager.setOnPageChangeListener(new ViewPager.OnPageChangeListener() {

            @Override
            public void onPageSelected(int position) {
                actionBar.setSelectedNavigationItem(position);
            }

            @Override
            public void onPageScrolled(int arg0, float arg1, int arg2) {
            }

            @Override
            public void onPageScrollStateChanged(int arg0) {
            }
        });
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        getMenuInflater().inflate(R.menu.menu_main, menu);
        return true;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        // Handle action bar item clicks here
        int id = item.getItemId();

        if (id == R.id.action_settings) {
            return true;
        }

        if (item.getItemId() == android.R.id.home) {
            NavUtils.navigateUpFromSameTask(this);
            return true;
        }

        return super.onOptionsItemSelected(item);
    }

    @Override
    public void onTabReselected(ActionBar.Tab tab, FragmentTransaction ft) {
    }

    @Override
    public void onTabSelected(ActionBar.Tab tab, FragmentTransaction ft) {
        // on tab selected show appropriate fragment
        viewPager.setCurrentItem(tab.getPosition());
    }

    @Override
    public void onTabUnselected(ActionBar.Tab tab, FragmentTransaction ft) {
    }

    @Override
    public void onStop(){
        super.onStop();
        Log.d("mytag", "MainActivity: onStop entered");
    }
}


TabsPageAdapter.java

package com.mydom.demoapp.adapter;

import android.support.v4.app.Fragment;
import android.support.v4.app.FragmentManager;
import android.support.v4.app.FragmentPagerAdapter;
import com.mydom.demoapp.MusicFragment;
import com.mydom.demoapp.MoviesFragment;
import com.mydom.demoapp.NewsFragment;

// This adapter provides fragment views to tabs.

public class TabsPagerAdapter extends FragmentPagerAdapter{

    public TabsPagerAdapter(FragmentManager fm) {
        super(fm);
    }

    @Override
    public Fragment getItem(int index) {

        switch (index) {
            case 0:
                return new MusicFragment();
            case 1:
                return new MoviesFragment();
            case 2:
                return new NewsFragment();
        }
        return null;
    }

    @Override
    public int getCount() {
        // get item count - equal to number of tabs
        return 3;
    }

}

MusicFragment.java (responsible for instantiating and launching an AsyncTask)

package com.mydom.demoapp;

import android.net.Uri;
import android.os.Bundle;
import android.support.v4.app.Fragment;
import android.util.Log;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;

import com.mydom.demoapp.async_task.AsyncTaskRunner;


public class MusicFragment extends Fragment {

    private static final String ARG_PARAM1 = "param1";
    private static final String ARG_PARAM2 = "param2";

    private String mParam1;
    private String mParam2;

    private OnFragmentInteractionListener mListener;

    public static MusicFragment newInstance(String param1, String param2) {
        MusicFragment fragment = new MusicFragment();
        Bundle args = new Bundle();
        args.putString(ARG_PARAM1, param1);
        args.putString(ARG_PARAM2, param2);
        fragment.setArguments(args);
        return fragment;
    }


    public MusicFragment() {
        // Required empty public constructor
    }

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        if (getArguments() != null) {
            mParam1 = getArguments().getString(ARG_PARAM1);
            mParam2 = getArguments().getString(ARG_PARAM2);
        }

        Log.d("mytag", "MusicFragment: onCreate entered");

    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

        // Inflate the layout for this fragment
        Log.d("janfry", "MusicFragment: onCreateView entered");
        return inflater.inflate(R.layout.fragment_music, container, false);
    }


    @Override
    public void onViewCreated(View view, Bundle savedInstanceState) {

        super.onViewCreated(view, savedInstanceState);

        Log.d("mytag", "MusicFragment: onViewCreated entered");

        runner = new AsyncTaskRunner();
        runner.execute(this.getActivity(), view);               
        // I am passing it the context (by getting the activity) and the view so that it will know where to update the UI.

    }

    public void onActivityCreated(Bundle savedInstanceState) {
        super.onActivityCreated(savedInstanceState);
    }

    // TODO: Rename method, update argument and hook method into UI event
    public void onButtonPressed(Uri uri) {
        if (mListener != null) {
            mListener.onFragmentInteraction(uri);
        }
    }

    public interface OnFragmentInteractionListener {
        // TODO: Update argument type and name
        public void onFragmentInteraction(Uri uri);
    }

    @Override
    public void onStop(){
        super.onStop();
        Log.d("mytag", "MusicFragment: onStop entered");
     }
}

AsyncTaskRunner.java

package com.mydom.demoapp.async_task;

import android.content.Context;
import android.graphics.Color;
import android.os.AsyncTask;
import android.util.Log;
import android.view.View;
import android.widget.TableLayout;
import android.widget.TableRow;
import android.widget.TextView;
import com.mydom.demoapp.R;
import com.mydom.demoapp.Utils;
import org.apache.http.HttpResponse;
// import org.apache.http.client.ClientProtocolException;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
// import org.apache.http.conn.ConnectTimeoutException;
import org.apache.http.impl.client.DefaultHttpClient;
import org.apache.http.params.BasicHttpParams;
import org.apache.http.params.HttpConnectionParams;
import org.apache.http.params.HttpParams;
import org.apache.http.util.EntityUtils;
import org.json.JSONArray;
// import org.json.JSONException;
import org.json.JSONObject;

// import java.io.IOException;
// import java.net.SocketTimeoutException;
// import java.util.concurrent.TimeoutException;


public class AsyncTaskRunner extends AsyncTask<Object, String, JSONArray> {

    // These will be set by doInBackground() according to what the fragment passed to it
    // I am declaring them as instance variables because I'll need them in the onPostExecute method too, so to have a ref to the frag to update. 
    // By the way, can I infer one from the other someway?

    Context contextRef;
    View viewRef;

    @Override
    protected void onPreExecute(){
        Log.d("janfry", "AsyncTaskRunner: onPreExecute entered");
    }

    @Override
    protected JSONArray doInBackground(Object... params){

        Log.d("mytag", "AsyncTaskRunner: doInBackground entered");

        contextRef = (Context) params[0]; 
        viewRef = (View) params[1];

        HttpResponse response;
        String str = "";
        JSONArray arrayOfJSONObjects = null;

        final HttpParams httpParams = new BasicHttpParams();
        HttpConnectionParams.setConnectionTimeout(httpParams, 5000);
        HttpClient myClient = new DefaultHttpClient(httpParams);
        HttpGet myConnection = new HttpGet("http://123.123.123.123:80/something.json");

        try {
            response = myClient.execute(myConnection);
            str = EntityUtils.toString(response.getEntity(), "UTF-8");
        } catch (Throwable t) {
            t.printStackTrace();
        }

        try{
            arrayOfJSONObjects = new JSONArray(str);
        } catch ( Throwable t) { t.printStackTrace(); }


        try {
            Log.d("mytag", arrayOfJSONObjects.getString(0));
        } catch (Throwable t) {
            t.printStackTrace();
        }
        return arrayOfJSONObjects;
    }


    @Override
    protected void onProgressUpdate(String... notused){

    }

    @Override
    protected void onPostExecute(JSONArray arrayOfJSONobjs) {


        Log.d("mytag", "AsyncTaskRunner: onPostExecute entered");

        TableLayout tab_lay = (TableLayout) viewRef.findViewById(R.id.musicTableLayout);
        tab_lay.removeAllViews();

        TextView[] arrayOfTextViews;
        arrayOfTextViews = new TextView[arrayOfJSONobjs.length()];    

        for(int pos = 0; pos < arrayOfJSONobjs.length(); pos++) {             
            // and let's populate it with textviews...
            TextView textViewForObjName = new TextView(contextRef);
            try {
                JSONObject oneJsonObj;   // will hold the parsed JSON for one obj
                oneJsonObj = arrayOfJSONobjs.getJSONObject(pos);
                textViewForObjName.setText(oneJsonObj.getString("name"));

            } catch (Throwable t) {
                t.printStackTrace();
            }

            textViewForObjName.setHeight(Utils.dip(contextRef, 30));
            textViewForObjName.setBackgroundColor(Color.parseColor("#ABCABC"));

            // let's add the text_view we built to the Array
            arrayOfTextViews[pos] = textViewForObjName;  


        }    // we now have an array of textviews, that has not been added to the UI yet.


        // I want to populate the array with 3 textviews per row.
        // Is it a good idea to use this layout for this way of laying out content?
        // Would you have done that differently?

        TableRow table_row = new TableRow(contextRef);
        int col_counter = 0;
        for (TextView aTextView : arrayOfTextViews) {

            table_row.addView(aTextView);
            col_counter++;

            if (col_counter == 3) {
                tab_lay.addView(table_row);
                table_row = new TableRow(contextRef);
                col_counter = 0;
            }
        }

    }


}
\$\endgroup\$

Your Answer

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

Browse other questions tagged or ask your own question.