5
\$\begingroup\$

I have this class:

public class MapController
{
    private Polyline _currentPolyline; // The line connecting two locations on the map
    private GoogleMap _map;
    private Context _context;

    public MapController(GoogleMap map, Context context)
    {
        _map = map;
        _context = context;
    }

    /**
     * Draws a route on the map.
     *
     * @param directions the Google Map Directions JSON string with the routes
     * @throws JSONException
     */
    public void drawRoute(String directions) throws JSONException
    {
        // First parse the JSON to extract the points into an easily manipulated form
        List<List<HashMap<String, String>>> routes = new DirectionsHelper().parsePathsFromJson(directions);

        // Only one polyline can be shown on the map at a time
        hideRoute();

        ArrayList<LatLng> points;
        PolylineOptions polyLineOptions = null;

        // Traversing through routes
        for (int i = 0; i < routes.size(); i++)
        {
            points = new ArrayList<LatLng>();
            polyLineOptions = new PolylineOptions();
            List<HashMap<String, String>> path = routes.get(i);

            for (int j = 0; j < path.size(); j++)
            {
                HashMap<String, String> point = path.get(j);

                double lat = Double.parseDouble(point.get("lat"));
                double lng = Double.parseDouble(point.get("lng"));
                LatLng position = new LatLng(lat, lng);

                points.add(position);
            }

            polyLineOptions.addAll(points);
            polyLineOptions.color(ContextCompat.getColor(_context, R.color.colorPrimary));
        }

        _currentPolyline =  _map.addPolyline(polyLineOptions);
    }

    /**
     * Removes the currently drawn {@link Polyline} from the map.
     */
    public void hideRoute()
    {
        if (_currentPolyline != null)
        {
            _currentPolyline.removeRoute();
        }
    }
}

Is it generally a good practice to call hideRoute() inside the drawRoute() method or does it depend on other factors? For example: should the caller be responsible for clearing the map before drawing the new path or does the caller not care?

\$\endgroup\$
  • 2
    \$\begingroup\$ When you say clearRoute() do you mean hideRoute()? I don't see clearRoute() in the code. \$\endgroup\$ – vnp Mar 18 '16 at 0:33
1
\$\begingroup\$

From an algorithm flow viewpoint, it looks good. However, I'd make a few small changes:

public void drawRoute(String directions) throws JSONException
{
    // First parse the JSON to extract the points into an easily manipulated form
    List<List<HashMap<String, String>>> routes = new DirectionsHelper().parsePathsFromJson(directions);

    // Only one polyline can be shown on the map at a time
    hideRoute();

    ArrayList<LatLng> points;
    PolylineOptions polyLineOptions = null;

    // Traversing through routes
    for (int i = 0; i < routes.size(); i++)
    {
        points = new ArrayList<LatLng>();
        polyLineOptions = new PolylineOptions();
        List<HashMap<String, String>> path = routes.get(i);

        for (int j = 0; j < path.size(); j++)
        {
            HashMap<String, String> point = path.get(j);

            double lat = Double.parseDouble(point.get("lat"));
            double lng = Double.parseDouble(point.get("lng"));
            LatLng position = new LatLng(lat, lng);

            points.add(position);
        }

        polyLineOptions.addAll(points);
        polyLineOptions.color(ContextCompat.getColor(_context, R.color.colorPrimary));
    }

    _currentPolyline =  _map.addPolyline(polyLineOptions);
}

In drawRoute, you first declare polyLineOptions = new PolylineOptions();, and then after the for loop, you assign values.

I think it'd be better if you moved the creation of PolylineOptions so that it's created and used in one place:

 public void drawRoute(String directions) throws JSONException
{
    // First parse the JSON to extract the points into an easily manipulated form
    List<List<HashMap<String, String>>> routes = new DirectionsHelper().parsePathsFromJson(directions);

    // Only one polyline can be shown on the map at a time
    hideRoute();

    ArrayList<LatLng> points;
    PolylineOptions polyLineOptions = null;

    // Traversing through routes
    for (int i = 0; i < routes.size(); i++)
    {
        points = new ArrayList<LatLng>();
        List<HashMap<String, String>> path = routes.get(i);

        for (int j = 0; j < path.size(); j++)
        {
            HashMap<String, String> point = path.get(j);

            double lat = Double.parseDouble(point.get("lat"));
            double lng = Double.parseDouble(point.get("lng"));
            LatLng position = new LatLng(lat, lng);

            points.add(position);
        }

        polyLineOptions = new PolylineOptions();
        polyLineOptions.addAll(points);
        polyLineOptions.color(ContextCompat.getColor(_context, R.color.colorPrimary));
    }

    _currentPolyline =  _map.addPolyline(polyLineOptions);
}

And for the hideRoute function,

public void hideRoute()
{
    if (_currentPolyline != null)
    {
        _currentPolyline.removeRoute();
    }
}

You should set _currentPolyline to null so you can't remove it twice (if that matters), plus it allows the GC to clean up the polyline.

\$\endgroup\$
  • \$\begingroup\$ So, you have basically answered that it's ok to clear the Polyline before drawing the routes inside the drawing method. Plus, a small improvement about instantiation of PolylineOptions. I don't need to GC the Polyline, because the MapController instance is associated with another Singleton class and destroyed when the application exits. So, overall it looks superb! Thank you! \$\endgroup\$ – iCantSeeSharp Mar 18 '16 at 13:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.