Google Maps API - reading locations from file and displaying markers on map

Question

I'm working with Google Maps and want to read in location objects into the map. I'm loading them in from a .csv file and create Shelter objects for each location that I save in an ArrayList. To actually add them to the map I iterate through this ArrayList and create marker objects with each object's data. Each actual Marker object created is also saved into another ArrayList so that I can reference them later (show/hide them on map when wanted). I feel like I'm doing something wrong. I included the relevant methods below.

private ArrayList<ShelterObject> shelterObjects;
private ArrayList<Marker> shelterMarkers;

// Generates the Shelter objects from the csv file and adds them to the  ArrayList.
private void generateShelterObjects()
{
    shelterObjects = new ArrayList<>();

    Log.i("SHELTER_TEST", "METHOD generateShelterObjects() STARTED");

    InputStream is = getResources().openRawResource(R.raw.shelters_csv_file);
    BufferedReader reader = new BufferedReader(
            new InputStreamReader(is, Charset.forName("UTF-8")));
    String line = "";

    try {
        while ((line = reader.readLine()) != null) {
            // Split the line into different tokens (using the comma as a separator).
            String[] tokens = line.split(",");

            String address = tokens[0];
            double latitude = Double.parseDouble(tokens[1]);
            double longitude = Double.parseDouble(tokens[2]);
            int numberOfOccupants = Integer.parseInt(tokens[3]);

            ShelterObject object = new ShelterObject(address, latitude, longitude, numberOfOccupants);
            shelterObjects.add(object);

        }

    } catch (IOException e1) {
        Log.e("MapsActivity2", "Error" + line, e1);
        e1.printStackTrace();
    }
    Log.i("SHELTER_TEST", "The size of the arraylist after reading in objects: " + shelterObjects.size());
}

// Iterates through the ArrayList of Shelter objects and adds a marker with data from that object to the map. The markers are saved in another ArrayList so they can be referenced later.
private void addShelterMarkersToMap()
{
    shelterMarkers = new ArrayList<>();

    for (ShelterObject obj : shelterObjects) {
        LatLng latLng = new LatLng(obj.getLatitude(), obj.getLongitude());
        Marker marker = mMap.addMarker(new MarkerOptions()
                .position(latLng)
                .title(obj.getAddress())
                .snippet("Antal platser: " + obj.getNumberOfOccupants()));
        shelterMarkers.add(marker);
    }
}

Answer 1

1. Your Marker and ShelterObject instances have the same data inside. (except the additional hardcoded (!) string "Antal platser:", whatever that means) Now you have two islands of data. You collect them in two different lists, shelterObjects and shelterMarkers. That is bad, High memory usage and difficult modifications are the drawback. If you want to add/delete/modify one you have to to it in two lists and keep it synchronized. I recommend to only have one island of data (ShelterObjects-array) and whenever you need ShelterMarkers, you convert them. For example you can add a method getAsMarker() to your ShelterObject class that returns itself as Marker. If you have enough memory and want to save performance you only create them when needed and store them for later re-use(see Lazy Instantiation), like so

   class ShelterObject {
      ...
      int Marker marker;
      public Marker getAsMarker(... mMap) { // or pass mMap in constructor. 
         if (marker != null) {
            return marker;
         }
         LatLng geoPosition = new LatLng(this.getLatitude(), this.getLongitude());
         marker = mMap.addMarker(...); // creates marker via MarkerOptions
         return marker;
      }
      ...
   }
   

2. What is "LatLng"? You barely can speak that out. If you mean just a pair of latitude and longitude coordinates, then just name it like that: geoPosition, coordinates, latitudeAndLongitude. Try to use short and meaningful names, but no abbreviations. In old times, even Microsoft used code abbreviations like "lptcstr" (google it). Because of bad experience, this is discouraged now. I can't count how many times I have looked up which combination of manager and number the managerNumber was abbreviated: "num", "nbr", "no", "numero", "man", "mgr", "mangr" etc.

3. Delete the code line below. You overwrite it right away, so you can leave it out for less code clutter.

   String line = "";

4. You forgot to close your stream in a finally-statement of your try-block. This is a severe bug since it's hard to detect when files remain open and suddenly your program crashes when it runs out of file handles. Alternatively you can use a try-with-resources statement.

5. Avoid using member variables like they are global. Just pass them as parameters. You can better test it and make your code more stable. I will give you an example what I mean: Assume you have following lines inside your init-method:

   generateShelterObjects(); addShelterMarkersToMap();

Now somebody changes it to following code that crashes or does not add the shelterMarkers:

addShelterMarkersToMap();
generateShelterObjects();

Does he know that he is not allowed to swap the lines when he needs to refactor? Maybe he does it by accident?

You can avoid that by coding:

ArrayList<ShelterObject> newShelterObjects = generateShelterObjects();
ArrayList<Markers> newShelterMarkers = addShelterMarkersToMap(shelterObjects);
shelterMarkers = newShelterMarkers;
shelterObjects = newShelterObjects;

Now when you swap the first two lines, it will not compile anymore. Also there is no possibility that you can have a nullPointerException in addShelterMarkersToMap() anymore. (I assume that you return an empty Map instead of null in generateShelterObjects() or throw an exception there). You also cannot have inconsistent data anymore. Just imagine in generateShelterObjects() your map is half-filled when an error occurs. Or your markers are half created when an error occurs. You need to catch the exception and clean both maps. Having inconsistent data is worse than just keeping the old, consistent ones.

6. Use "/** comment **/" instead of "// comment" on top of your methods, so you can create javadocs easily and the IDE can show it as mouseover-message