Extracting Android contact Info

I am new to Android programming and I'm working on an Activity in an app that will plot the location of contacts from a specific email account on to a map. The following is what I in a class that the Activity will use to get the Contact info I want to use when plotting the location. I have tested this and it works. But as I researched the possible ways to pull this off (and how to program with Android in general). I have found that best practice is to optimize your code. I have two questions I want to ask.

First, is there a more optimized way of getting the name, full address, and geopoints of a contact from a specific email account (like an exchange email)?

my class file:

```
import java.util.ArrayList;
import java.util.List;
import com.google.android.maps.GeoPoint;

import android.database.Cursor;
import android.location.Address;
import android.location.Geocoder;
import android.provider.ContactsContract;
import android.content.ContentResolver;
import android.content.Context;

public class Contacts {
ArrayList nameList = new ArrayList();
ArrayList locList = new ArrayList();
ArrayList geoList = new ArrayList();
Context context = null;
Geocoder gc;

private String getContactName(String id){
String name = null;

ContentResolver nmCr = context.getContentResolver();
String selection = ContactsContract.Contacts._ID + " = ?";
String[] selectionArgs = new String[]{id};
String[] projection = new String[]{
ContactsContract.Contacts.DISPLAY_NAME
};
Cursor nmCur = nmCr.query(ContactsContract.Contacts.CONTENT_URI,
projection, selection, selectionArgs, null);
if (nmCur.getCount() > 0)
{
while (nmCur.moveToNext())
{
//if the account is under com.android.exchange and not com.google
// Pick out the ID, and the Display name of the

Just some generic Java notes since I'm not too familiar with Android.

-
The reference type of your list should be only List. Instead of:

ArrayList  nameList = new ArrayList();

use:

List  nameList = new ArrayList();

Type List vs type ArrayList in Java

-
Setting context to null looks unnecessary, since null is the default value.

Context context = null;

-
You should close the Cursor in a finally block:

Cursor nmCur = nmCr.query(...);
try {
    ...
} finally {
    nmCur.close();
}

It will help to avoid resource leaks. (When the code throws an exception close won't be called.)

-
I'd use guard clauses to check the return value of the query. It makes the code flatten and easier to read.

if (nmCur.getCount() <= 0) {
    return null;
}

References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

-
If I'm right, you use just the last result of the query in the getContactName method. It looks unnecessary to read all results in the while loop:

while (nmCur.moveToNext()) {
    name = nmCur.getString(nmCur.getColumnIndex(
                    ContactsContract.Contacts.DISPLAY_NAME));
}

Anyway, you could move the getColumnIndex calls out of the loop which should be faster:

final int displayNameIndex = nmCur.getColumnIndex(ContactsContract.Contacts.DISPLAY_NAME)
while (nmCur.moveToNext()) {
    name = nmCur.getString(displayNameIndex);
}

(The latter is true for the getContactLocations method too.)

-
In the getContactLocations method addr should be a StringBuilder instead of String. You concatenate Strings in a loop.

(I don't know, maybe the compiler change/optimize it for you.)

-
This:

Geocoder gc;

should be a local variable in the getContactGeo method, since other methods don't use this reference.

-
Instead of magic number 5 use a named constants or variable:

final int maxResults = 5;
List foundAdresses =  gc.getFromLocationName(addr, maxResults);

It helps readers a lot to figure out what the code should do without checking the javadoc.

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

-
Maybe you should create a constructor with a Context parameter since other methods are unusable without a reference to a Context instance.

private final Context context;

public Contacts(final Context context) {
    if (context == null) {
        throw new NullPointerException("context cannot be null");
    }
    this.context = context;
}

Anyway, the methods currently temporally coupled (need to be called in a specific order) which could be confusing to the clients and lead to NullPointerExceptions.

-
Fields should be private:

private List nameList = new ArrayList();
private List locList = new ArrayList();
private List geoList = new ArrayList();

Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.

-
This line doesn't smell good:

overlayitem = new OverlayItem(c.geoList.get(i),c.nameList.get(i) ,c.locList.get(i));

Maybe the getContactNames should create the list of OverlayItem objects instead of the three lists whose are accessed with the same index.

StackExchange Code Review Q#7465, answer score: 6