Optimization of Android code suggestions? Iteration based

Question

At the "if(a==2)" part of my code below, the android emulator takes 8 seconds to process and display some data in a list. I need to reduce this time to 1 second or less. I am accessing a 50 KB .txt file with 3200 lines, comparing each line with a string passed by another function, and whichever line matches the string, I am printing that in a list. The format of data in the .txt file is like this:

0,1>Autauga;     
0,2>Baldwin;      
0,3>Barbour;         
1,69>Aleutians East;      
1,68>Aleutians West;

etc... and it goes on for 3200 lines. The number before the comma is compared to the string I passed from another function. If they match, I print the line. Here is the code:

package com.example.countylists;

import java.io.BufferedInputStream;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.StringTokenizer;

import android.app.Activity;
import android.content.Intent;
import android.os.Bundle;
import android.util.Log;
import android.view.View;
import android.widget.AdapterView;
import android.widget.ListView;
import android.widget.SimpleAdapter;

public class ListViewA extends Activity{

    List<HashMap<String, String>> fillMaps = new ArrayList<HashMap<String, String>>();
    int a=1;//stores instance number of the list
    /** Called when the activity is first created. */
    @Override    
public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);
        ListView lv= (ListView)findViewById(R.id.listview);
        String newString;//item clicked in previous list
        int instanceposition;//tells me which instance number is running
        int position=0;//position of the click
        Bundle extras;
        if (savedInstanceState == null) {
            extras = getIntent().getExtras();
            if(extras == null) {
                newString= null;
                instanceposition=1;
                Log.d(Integer.toString(instanceposition),"value of instanceposition");
                a=instanceposition;
                position=0;

            } else {
                newString= extras.getString("Key");
                instanceposition=extras.getInt("instpos");
                Log.d(Integer.toString(instanceposition),"value of instanceposition");
                a=instanceposition;
                position=extras.getInt("position");

            }
        } else {
            newString= (String) savedInstanceState.getSerializable("Key");
        }

        // create the grid item mapping
        String[] from = new String[] {"col_1"};
        int[] to = new int[] {R.id.item2};
        String[] array;
        Log.d(Integer.toString(a),"value of a");
//list of states
        if(a==1)
        {   String z="";
            try{
                InputStream is = getAssets().open("USSTATES.txt");
                InputStreamReader iz=new InputStreamReader(is);
                BufferedReader bis = new BufferedReader(iz);

            int v=0;
            v=count("USSTATES.txt");
            Log.d(Integer.toString(v),"value of v");
            array=new String[v];
        for(int i=0;i<v;i++){
            z=bis.readLine();
            array[i]=z;
        }
     // prepare the list of all records
        for(int q = 0; q <v; q++){
            HashMap<String, String> map = new HashMap<String, String>();
            map.put("col_1", array[q]);
            fillMaps.add(map);
            lv.setOnItemClickListener(onListClick);
        }
        }catch(Exception E){E.printStackTrace();
}
        }
        else if(a==2){
            String z="";
            try{
                InputStream is = getAssets().open("USCOUNTIES.txt");
                InputStreamReader iz=new InputStreamReader(is);
                BufferedReader bis = new BufferedReader(iz);

                int v=0;
                //finding no. of counties to be displayed
                v=count("USCOUNTIES.txt");

                int counter=0;
                String pos;
                pos=Integer.toString(position);
                try{
                for(int i=0;i<v;i++){
                    z=bis.readLine();
                    //int x=pos.length();
                    boolean a;
                    //using stringtokenizer
                    StringTokenizer st = new StringTokenizer(z, ","); 
                    String substring;
                    substring=(String) st.nextElement();
                    a=substring.equals(pos);
                    if(a==true){

                        counter=counter+1;

                    }
                }}catch(Exception e){e.printStackTrace();}
                String array1[]=new String[counter];

                try{
                    InputStream ig = getAssets().open("USCOUNTIES.txt");
                    InputStreamReader ia=new InputStreamReader(ig);
                    BufferedReader bos = new BufferedReader(ia);
                int j=0;
                for(int i=0;i<v;i++){
                    z=bos.readLine();
                    Log.d(z,"Value of zyo");
                    String[] split = z.split(",");
                    if(split[0].equals(pos)){
                        array1[j]=split[1];
                        j=j+1;
                    }

                }}
                catch(Exception e){e.printStackTrace();}

            Log.d(Integer.toString(v),"value of v(when a is 1)");

     // prepare the list of all records
        for(int q = 0; q <v; q++){
            HashMap<String, String> map = new HashMap<String, String>();
            map.put("col_1", array1[q]);
            fillMaps.add(map);
            lv.setOnItemClickListener(onListClick);
        }
        }catch(Exception E){E.printStackTrace();
}   
        }

        a=a+1;
        // fill in the grid_item layout
        SimpleAdapter adapter = new SimpleAdapter(this, fillMaps, R.layout.grid_item, from, to);
        lv.setAdapter(adapter);
    }
    private AdapterView.OnItemClickListener onListClick=new AdapterView.OnItemClickListener(){
    public void onItemClick(AdapterView<?> parent,View view, int position, long id)
    {
        Intent i=new Intent(ListViewA.this,ListViewA.class);
        i.putExtra("Key",fillMaps.get(position));
        i.putExtra("instpos", a);
        i.putExtra("position",position);
        startActivity(i);
    }
};
public int count(String filename) throws IOException {
    InputStream is = getAssets().open(filename);
    BufferedInputStream bis = new BufferedInputStream(is);
    try {
        byte[] c = new byte[1024];
        int count = 0;
        int readChars = 0;
        boolean empty = true;
        while ((readChars = bis.read(c)) != -1) {
            empty = false;
            for (int i = 0; i < readChars; ++i) {
                if (c[i] == '\n') {
                    ++count;
                }
            }
        }
        return (count == 0 && !empty) ? 1 : count+1;
    } finally {
        bis.close();
    }
}

Answer 1

1. These streams are never closed:

   InputStream is = getAssets().open("USSTATES.txt");
   InputStreamReader iz = new InputStreamReader(is);
   BufferedReader bis = new BufferedReader(iz);
   

   You should close them in a finally block or use try-with-resources. See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

2. (Refactored code)

   String line = bis.readLine();
   StringTokenizer st = new StringTokenizer(line, ",");
   String substring = (String) st.nextElement();
   boolean a = substring.equals(pos);
   if (a == true) {
       counter = counter + 1;
   }
   

   The code does not use too much from the StringTokenizer. The following is the same:

   final boolean a = line.startsWith(pos + ",");
   if (a) {
       counter++;
   }
   

   I guess it might be faster.

3. String array1[] = new String[counter];
   

   You could use an ArrayList which doesn't need a size on creation (it grows if you add elements to it), so you could eliminate the file reading which counts the lines as well as the second loop (including the second file reading) which counts the matching lines just to create a suitable sized array which will be used in the third loop.

4. String[] split = z.split(",");
   if (split[0].equals(pos)) {
       array1.add(split[1]);
       j = j + 1;
   }
   

   Set the limit parameter of split if you are not using all values of the result array:

   final String[] split = z.split(",", 3);
   if (split[0].equals(pos)) {
       array1.add(split[1]);
       j = j + 1;
   }
   

   It could improve performance.

5. public int count(String filename) throws IOException {
   

   A more descriptive method name would be better. What does this method count? Put it into the method name! I guess countLines would be fine.

6. Short variable names are hard to read:

   byte[] c = new byte[1024];
   

   I suppose you have autocomplete, so using longer names does not mean more typing but it would help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent. (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

   buffer would do it.

7. If there is an error you should handle it, or maybe show an error message to the user instead of the printStackTrace:

      } catch (Exception e) {
          e.printStackTrace();
      }
   

   See also:

   • It isn't the best idea to use printStackTrace() in Android exceptions
   • Avoid printStackTrace(); use a logger call instead
   • Why is exception.printStackTrace() considered bad practice?

8. As others already mentioned, the onCreate method is too complex (and long). You should break it to separate methods with good names which explain their intent. Comments usually help, sometimes they are perfect method names.

9. This variable is write-only, the code never read its value so it's superfluous:

   String newString;// item clicked in previous list
   

   Eclipse shows it with a yellow warning.

10. You could eliminate the comment with better variable naming:

     int instanceposition;// tells me which instance number is running
     int position = 0;// position of the click 
    

    Just rename them to runningInstanceNumber and clickPosition.

11. This runningInstanceNumbe variable scope could be smaller:

    if (extras == null) {
        final int runningInstanceNumber = 1;
        Log.d(Integer.toString(runningInstanceNumber), "value of instanceposition");
        a = runningInstanceNumber;
        position = 0;
    } else {
        final int runningInstanceNumber = extras.getInt("instpos");
        Log.d(Integer.toString(runningInstanceNumber), "value of instanceposition");
        a = runningInstanceNumber;
        position = extras.getInt("position");
    
    }
    

    (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

12. Widening its scope a little bit (but it's still smaller than in the original code) you could move the logging outside the if to remove some duplication:

    final int runningInstanceNumber;
    if (extras == null) {
        runningInstanceNumber = 1;
        position = 0;
    } else {
        runningInstanceNumber = extras.getInt("instpos");
        position = extras.getInt("position");
    }
    a = runningInstanceNumber;
    Log.d(Integer.toString(runningInstanceNumber), "value of instanceposition");
    

13. int v = 0;
    v = count("USSTATES.txt");
    

    The following is the same:

    int v = count("USSTATES.txt");
    

14. List<HashMap<String, String>> fillMaps =
        new ArrayList<HashMap<String, String>>();
    

     

    HashMap<String, String> map = new HashMap<String, String>();
    

    HashMap<...> reference types should be simply Map<...>, as well as ArrayList<...> references could be List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

    Map<String, String> map = new HashMap<String, String>();
    

     

    List<Map<String, String>> fillMaps =
        new ArrayList<Map<String, String>>();
    

Here is the a == 2 branch after the refactoring mentioned above (nontested code) with only one file reading and file iteration:

// finding no. of counties to be displayed
final int lines = count("USCOUNTIES.txt");
final String pos = Integer.toString(clickPosition);
final List<String> machingValues = new ArrayList<String>();
final InputStream countiesStream = getAssets().open("USCOUNTIES.txt");
final InputStreamReader countesReader = new InputStreamReader(countiesStream);
final BufferedReader bufferedCountiesReader = new BufferedReader(countesReader);
try {
    while (true) {
        final String line = bufferedCountiesReader.readLine();
        if (line == null) {
            break;
        }
        Log.d(line, "Value of zyo");
        final String[] split = line.split(",", 3);
        if (split[0].equals(pos)) {
            machingValues.add(split[1]);
        }

    }
} catch (final Exception e) {
    e.printStackTrace();
} finally {
    bufferedCountiesReader.close();
}

Log.d(Integer.toString(lines), "value of v(when a is 1)");

// prepare the list of all records
for (final String value: machingValues) {
    final Map<String, String> map = new HashMap<String, String>();
    map.put("col_1", value);
    fillMaps.add(map);
    lv.setOnItemClickListener(onListClick);
}