Extracting Android contact Info

Question

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<String> nameList = new ArrayList<String>();
 ArrayList<String> locList = new ArrayList<String>();
 ArrayList<GeoPoint> geoList = new ArrayList<GeoPoint>();
 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
 // contact from the current row of the cursor
 name = nmCur.getString(nmCur.getColumnIndex(
 ContactsContract.Contacts.DISPLAY_NAME));
 }
 }
 nmCur.close();
 return name;
 }
 public String getContactLocations(String id){
 String addr = null;
 ContentResolver addrCr = context.getContentResolver();
 String selection = ContactsContract.CommonDataKinds.StructuredPostal.CONTACT_ID + " = ?"; 
 String[] selectionArgs = new String[]{id}; 
 String[] projection = new String[]{
 ContactsContract.CommonDataKinds.StructuredPostal.STREET,
 ContactsContract.CommonDataKinds.StructuredPostal.CITY,
 ContactsContract.CommonDataKinds.StructuredPostal.REGION,
 ContactsContract.CommonDataKinds.StructuredPostal.POSTCODE
 }; 
 Cursor addrCur = addrCr.query(ContactsContract.CommonDataKinds.StructuredPostal.CONTENT_URI, 
 projection, selection, selectionArgs, null); 
 if (addrCur.getCount() > 0)
 {
 while(addrCur.moveToNext()) {
 addr = addrCur.getString(
 addrCur.getColumnIndex(ContactsContract.CommonDataKinds.StructuredPostal.STREET));
 addr += " " + addrCur.getString(
 addrCur.getColumnIndex(ContactsContract.CommonDataKinds.StructuredPostal.CITY));
 addr += " " + addrCur.getString(
 addrCur.getColumnIndex(ContactsContract.CommonDataKinds.StructuredPostal.REGION));
 addr += " " + addrCur.getString(
 addrCur.getColumnIndex(ContactsContract.CommonDataKinds.StructuredPostal.POSTCODE));
 }
 }
 addrCur.close();
 return addr;
 }
 public GeoPoint getContactGeo(String addr){
 GeoPoint p = null;
 gc = new Geocoder(context); //create new geocoder instance
 try {
 List<Address> foundAdresses = gc.getFromLocationName(addr, 5); //Search addresses
 for (int i = 0; i < foundAdresses.size(); ++i) {
 //Save results as Longitude and Latitude
 Address x = foundAdresses.get(i);
 p = new GeoPoint((int)(x.getLatitude() * 1E6),(int)(x.getLongitude() * 1E6));
 }
 }
 catch (Exception e) {
 e.printStackTrace();
 }
 return p;
 }
 public void getContactNames( Context context, String email){
 this.context = context;
 ContentResolver cr = context.getContentResolver();
 String[] projection = new String[]{
 ContactsContract.RawContacts.CONTACT_ID
 };
 String selection = ContactsContract.RawContacts.ACCOUNT_NAME + " = ? AND " + ContactsContract.RawContacts.ACCOUNT_TYPE + " = ?"; 
 String[] selectionArgs = new String[]{
 email,
 "com.android.exchange" 
 }; 
 Cursor cur = cr.query(ContactsContract.RawContacts.CONTENT_URI,
 projection, selection, selectionArgs, null);
 if (cur.getCount() > 0)
 {
 while (cur.moveToNext())
 {
 String id = cur.getString(cur.getColumnIndex(ContactsContract.RawContacts.CONTACT_ID));
 nameList.add(
 getContactName(id)
 );
 String addr = getContactLocations(id);
 locList.add(addr);
 geoList.add(getContactGeo(addr));
 }
 }
 cur.close();
 }
}

Second, right now, to use the data coming back from this file, I am calling this class's ArrayLists after executing c.getContactNames(this, email); in my Activity.

Looks like this:

c.getContactNames(this, sale.smtpEmail);
MapArray = MapView.getOverlays();
for (int i = 0; i < c.nameList.size(); i++) { 
OverlayItem overlayitem = null;
 drawable = new BitmapDrawable(mar.marker(i, this));
 itemizedOverlay = new MapsItemizedOverlay(drawable, this);
 if(c.locList.get(i) != null){
 overlayitem = new OverlayItem(c.geoList.get(i), c.nameList.get(i), c.locList.get(i));
 itemizedOverlay.addOverlay(overlayitem);
 MapArray.add(itemizedOverlay); 
 }
} 

Which I am doing so I create unnecessary objects. Is this best practice?

Answer 1

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

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

   ArrayList<String> nameList = new ArrayList<String>();
   

   use:

   List<String> nameList = new ArrayList<String>();
   

   Type List vs type ArrayList in Java

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

   Context context = null;
   

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

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

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

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

7. This:

   Geocoder gc;
   

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

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

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

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

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

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

11. Fields should be private:

    private List<String> nameList = new ArrayList<String>();
    private List<String> locList = new ArrayList<String>();
    private List<GeoPoint> geoList = new ArrayList<GeoPoint>();
    

    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.

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

• Chillie: Thank you for your feedback. However if I set my lists to private ( in no. 11), how do I access the data in them from my Activity class? They are in to different java files.
• palacsint: Just create a getter method. (Also check Item 39: Make defensive copies when needed in Effective Java Second Edition.)
• Chillie: Will do. Again thank you for your input it is much appreciated.

Answer 2

One other suggestion is that you combine the separate queries of the contact database into a single query to get the name and location. I believe that this would improve performance as there is overhead involved in calling out to the content provider so many times.