Gfrktyl

The purpose of this code is to create a GoogleMap using PHP, add markers to it according to Place objects from database, add infoWindows for these markers. While reading about Factory pattern I came up with the following solution. What are the parts that can be improved. Thank you.

Factory for creating markers:

class MarkerFactory

{

    public function create(Coordinate $position, array $options = )

    {

        $marker = new Marker($position);

        $marker->setOptions($options);



        return $marker;

    }

}

Factory for creating infoWindows. Isn't it violating any principles because of injected dependency?

class InfoWindowFactory

{

    private $twig;



    public function __construct(Twig_Environment $twig)

    {

        $this->twig = $twig;

    }



    public function create(Place $place, $editable = false)

    {

        return new InfoWindow($this->renderWindowContent($place, $editable));

    }



    protected function renderWindowContent(Place $place, $editable)

    {

        if ($editable) {

            $template = 'editable_infowindow.html.twig';

        } else {

            $template = 'static_infowindow.html.twig';

        }



        return $this->twig->render($template, ['place' => $place]);

    }

}

MapFactory that uses MarkerFactory and InfoWindowFactory:

class MapFactory

{

    private $markerFactory;



    private $infoWindowFactory;



    public function __construct(MarkerFactory $markerFactory, InfoWindowFactory $infoWindowFactory)

    {

        $this->markerFactory = $markerFactory;

        $this->infoWindowFactory = $infoWindowFactory;

    }



    public function create(Coordinate $mapCenter, array $places = , array $mapOptions = , $isEditableMap = false)

    {

        $map = new Map();



        $map->setCenter($mapCenter);

        $map->setMapOptions($mapOptions);



        $markers = ;



        foreach ($places as $place) {

            $position = new Coordinate($place->getLatitude(), $place->getLongitude());



            $marker = $this->markerFactory->create($position);

            $infoWindow = $this->infoWindowFactory->create($place, $isEditableMap);



            $marker->setInfoWindow($infoWindow);



            $markers = $marker;

        }



        $map->getOverlayManager()->setMarkers($markers);



        return $map;

    }

}

And the code example of usage:

$markerFactory = new MarkerFactory();

$infoWindowFactory = new InfoWindowFactory($this->get('twig'));

$mapFactory = new MapFactory($markerFactory, $infoWindowFactory);



$center = new Coordinate(55.117100, 23.940332);

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

$map = $mapFactory->create($center, $places, $options);

I think your conception of the factory pattern is a little off. Throughout your classes, you seem to want to deal with concrete factory objects, which doesn't seem to make sense. Typically, a factory would be invoked statically and would not store local references (i.e. in properties) to the dependencies it is acting upon.

Your MarkerFactory class is close, just needing to have create() method defined as static and possibly defining the class itself as abstract since there would not seem to be any reason to ever instantiate it.

But getting into your other classes, there seem to be issues with where logic lies. The InfoWindowFactory probably should not have a method such as renderWindowContent() on it. That would seem to be a method that should perhaps exist on the InfoWindow itself. Now, if the factory needs to call that method on a concrete window object to get it set up properly before returning it, that might be OK (though I might suggest the InfoWindow constructor is changed to trigger this action).

I would like to see InfoWindowFactory be something like:

namespace YourNamespace;



abstract class InfoWindowFactory

{

    public static final function create(

        Place $place,

        Twig_Environment $twig,

        $template

    ) {

        return new InfoWindow($place, $twig, $template);

    }

}

Note that we are pushing the render operation down into the InfoWindow object where it probably belongs. Also note that we have removed the hard-coded template names from this class. Maybe they truly belong here, in which case maybe they should be class constants rather than buried in a method. But, depending on how you are using these InfoWindows, perhaps this should be passed through as configuration.

My comments on MapFactory would be similar to those for the InfoWindowFactory. Namely, I would rather see a class like structured like this:

namespace YourNamespace;



use MarkerFactory;

use InfowindowFactory;



abstract class MapFactory

{



    public static final function create(Coordinate $mapCenter, array $places = , array $mapOptions = , $isEditableMap = false)

    {

        $map = new Map();



        $map->setCenter($mapCenter);

        $map->setMapOptions($mapOptions);



        $markers = ;



        foreach ($places as $place) {

            $position = new Coordinate($place->getLatitude(), $place->getLongitude());



            $marker = MarkerFactory::create($position);

            $infoWindow = InfoWindowFactory::create($place, $isEditableMap);



            $marker->setInfoWindow($infoWindow);



            $markers = $marker;

        }



        $map->getOverlayManager()->setMarkers($markers);



        return $map;

    }

}

A comment about validation...

You are doing a good job of trying to enforce typehints on your methods, however I do wonder if it might make sense for you to have classes defined which deal with say a collection of Place objects. These seem to be critical dependencies that can only be enforced to be an array via type-hinting, meaning that if you truly wanted to be able to validate that you have valid place objects in your array, you might have to go into undesirable approach of inspecting each array item to validate it. If instead of passing simple array, you passed PlaceCollection or similar, you could be ensured that everything in that collection is a valid Place object. Certainly, this adds yet another bit of code to the overall system, so really only you can determine if it is worthwhile to do this depending on how you are planning to use this code. But it is something to consider when these are key items you are working with (vs. say optional configurations or similar).

Do you need any error or exception handling here? Typically one of the things a factory would do is raise an exception if the dependencies that are passed are not valid (typehinting can do a lot of this for you) or to handle any underyling exceptions that might happen within method execution (i.e. can instantiation of a Map, InfoWindow, etc. object throw an exception?). I don't really see any consideration here in your code for error or edge conditions. It seems as if this is considering happy path only.