Return to Answer

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

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

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();
   }
   

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; }

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; }

1,

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

The reference type of your list should be only List:

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

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;
}

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

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

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,

Geocoder gc;

8,

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

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();
}

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;
}

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

12,

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

This line doesn't smell good. Maybe the getContactNames should create the list of OverlayItem objects instead of the three lists whose are accessed with the same index.

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

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;
   }
   

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

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

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;
   

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();
   }
   

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; }

11. Fields should be private:

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

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.