Optimization of Android code suggestions? Iteration based

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> fillMaps = new ArrayList>();
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;

-
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

-
(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.

-

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.

-

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.

-

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.

-
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.

-
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?

-
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.

-
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.

-
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.

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

-
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");

-

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

The following is the same:

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

-

List> fillMaps =
    new ArrayList>();

 

HashMap map = new HashMap();

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 map = new HashMap();

 

List> fillMaps =
    new ArrayList>();

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 Strin

StackExchange Code Review Q#23995, answer score: 6