2
\$\begingroup\$

I want to simplify this process of creating map, putting markers on it according to Place objects from my database, creating info windows. Using egeloen/ivory-google-map library, I've written a facade for this proccess.

class PlaceMap
{
    protected $map;

    protected $templating;

    protected $places = [];

    public function __construct(Map $map, \Twig_Environment $templating)
    {
        $this->map = $map;
        $this->templating = $templating;

        $this->map->setStylesheetOption('width', '100%');
        $this->map->setStylesheetOption('height', '90vh');
        $this->map->setCenter(new Coordinate(55.117100, 23.940332));
        $this->map->setMapOption('zoom', 7);
    }

    public function setPlaces(array $places)
    {
        $this->places = $places;
    }

    public function getMap()
    {
        $this->process();

        return $this->map;
    }

    protected function process()
    {
        $markers = [];
        $events = [];

        foreach ($this->places as $place) {
            $marker = $this->placeToMarker($place);
            $window = $this->getPlaceInfoWindow($place);

            $marker->setInfoWindow($window);

            $events = array_merge($events, $this->createMarkerClickEvents($marker, $window));
            $markers[] = $marker;
        }

        $this->getMarkerCluster()->setMarkers($markers);
        $this->map->getEventManager()->setDomEvents($events);

    }

    protected function placeToMarker(Place $place)
    {
        return new Marker(
            new Coordinate($place->getLatitude(), $place->getLongitude())
        );
    }

    protected function getPlaceInfoWindow(Place $place)
    {
        return new InfoWindow($this->getInfoWindowContent($place));
    }

    protected function getMarkerCluster()
    {
        $cluster = $this->map->getOverlayManager()->getMarkerCluster();
        $cluster->setType(MarkerClusterType::MARKER_CLUSTERER);
        $cluster->setOption('imagePath', 'https://github.com/googlemaps/js-marker-clusterer/raw/gh-pages/images/m');

        return $cluster;
    }

    protected function getInfoWindowContent(Place $place)
    {
        return $this->templating->render('AppBundle::place_infowindow.html.twig', ['place' => $place]);
    }

    protected function createMarkerClickEvents(Marker $marker, InfoWindow $window)
    {
        return [new Event($marker->getVariable(), MouseEvent::CLICK, $this->getMarkerClickHandler($window))];
    }

    protected function getMarkerClickHandler(InfoWindow $window)
    {
        return "function () {
            if (currentInfoWindow) {
                currentInfoWindow.close();
            }

            {$window->getVariable()}.open({$this->map->getVariable()}, this);

            currentInfoWindow = {$window->getVariable()};
        }";
    }
}

And I use it like this:

$placeMap = new PlaceMap(new Map(), $this->get('templating'));
$placeMap->setPlaces($places);

$mapInstance = $placeMap->getMap();

Is this a proper usecase to use a facade patter? Should I pass the instance of Map to the facade constructor? What about all the Marker, InfoWindow, Event objects, that I use inside a facade? Is it ok to instantiate them in the facade?

EDIT: According to Mike advices, I made a MapFactory, that is responsible for creating static/editable map from passed options.

class MapFactory
{
    const CLUSTER_IMAGE_URI = 'https://github.com/googlemaps/js-marker-clusterer/raw/gh-pages/images/m';

    public static function buildMap(Coordinate $mapCenter, array $places = [], array $mapOptions = [], $isEditableMap = false)
    {
        $map = new Map();

        $defaultOptions = [
            'zoom' => 7
        ];

        $mapOptions = array_merge($mapOptions, $defaultOptions);

        $map->setCenter($mapCenter);
        $map->setMapOptions($mapOptions);

        $markers = [];

        foreach ($places as $place) {
            $marker = new Marker(new Coordinate($place->getLatitude(), $place->getLongitude()));
            $marker->setOption('draggable', $isEditableMap);

            if ($isEditableMap) {
                // initiate events for editable markers, infowindow content for editable marker
            } else {
                // initiate events for static markers, infowindow content for static marker
            }

            $markers[] = $marker;
        }

        if ($isEditableMap) {
            $map->getOverlayManager()->setMarkers($markers);
        } else {
            $cluster = $map->getOverlayManager()->getMarkerCluster();
            $cluster->setType(MarkerClusterType::MARKER_CLUSTERER);
            $cluster->setOption('imagePath', self::CLUSTER_IMAGE_URI);

            $cluster->setMarkers($markers);
        }

        return $map;
    }
}

Use:

$center = new Coordinate(55.117100, 23.940332);
$places = $repository->findAll();

$map = MapFactory::buildMap($center, $places);

It looks like there is too much code in a factory method. Should I split some related part to other protected static methods? Or is it too much lifting for a factory to create map, initiate markers on it, sent infoWindows and event handlers?

While reading a articles on factory pattern I noticed, that they are used to instantiante multiple different type of objects and not to encapsulate logic.

\$\endgroup\$
1
\$\begingroup\$

Since Places is such a hard dependency in this process, I would consider passing that as a dependency rather than breaking this up across multiple steps. I would even question the need for whether PlaceMap needs to be a concrete class vs. a simple statically accessed factory.

Usage in this case might be like:

$mapInstance = PlaceMap::buildMap(
    new Map(),
    array $places,
    $this->get('templating')
);

This approach would allow you to remove setPlaces() and getMaps() methods such that you only maintain a single public interface for a single creation step.


public function __construct(Map $map, \Twig_Environment $templating)
{
    $this->map = $map;
    $this->templating = $templating;

    $this->map->setStylesheetOption('width', '100%');
    $this->map->setStylesheetOption('height', '90vh');
    $this->map->setCenter(new Coordinate(55.117100, 23.940332));
    $this->map->setMapOption('zoom', 7);
}

In your constructor, you are doing a lot of manipulations on the passed Map object. I would think that since you are passing this in as a dependency, it should already have it's default values set, eliminating need for this code here. You might also consider being able to pass in a config object, Coordinate object or similar to set as default location on map.

I don't like defaults hard-coded in this way. Perhaps defaults should be constants on class, or you simply require that config data be passed in.


protected function createMarkerClickEvents(Marker $marker, InfoWindow $window)
{
    return [new Event($marker->getVariable(), MouseEvent::CLICK, $this->getMarkerClickHandler($window))];
}

I question the method name and return signature here. You are only ever returning a single Event (though wrapped in array). Why not drop s from end of method name and remove the array wrapper on return to be less obscure about what is really happening here? This would also mean related change in process() method where you would replace array_merge() with more straightforward array push operation.


    return "function () {
        if (currentInfoWindow) {
            currentInfoWindow.close();
        }

        {$window->getVariable()}.open({$this->map->getVariable()}, this);

        currentInfoWindow = {$window->getVariable()};
    }";

Should this string template be either moved out of your class altogether into your display code, or perhaps at a minimum stored as default value on class?


protected function getInfoWindowContent(Place $place)
    {
        return $this->templating->render('AppBundle::place_infowindow.html.twig',

['place' => $place]); }

Perhaps renderInfoWindowContent()? get typically implies return of some value.


Would like to see some minimal comments in certain points, though code is pretty self-explanatory. Consider Doc blocks.

\$\endgroup\$
  • \$\begingroup\$ You suggest using factory pattern in this case? I have two classes PlaceMap and EditablePlaceMap. They differ in info window content, marker click event handler and some marker options. Is this usecase for factory? \$\endgroup\$ – andrius.k Nov 1 '16 at 13:51
  • \$\begingroup\$ @nn4n4s Probably. If that factory is geared towards accepting appropriate dependencies and generating instances of PlaceMap or its inheriting classes, I think this makes sense, \$\endgroup\$ – Mike Brant Nov 1 '16 at 14:27
  • \$\begingroup\$ I added a different approach \$\endgroup\$ – andrius.k Nov 2 '16 at 9:58

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.