Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After experimenting with different design approaches for the past two years, I've created a library for managing and processing dynamic content to be displayed on a single HTML view. This library is PSR-0, PSR-1 and PSR-2 compliant. I'm quite happy with this design but want to make sure it can stand excessive use.

It's simple enough until you start dealing with nested data. To give a quick summary, each single piece of content on a page can be dynamically changed depending on its associated GET parameter. This is what I've called a Slot.

For exmaple, on a page, I might have a Slot for a headline, which would be bound to Get parameter h, so ?h=3 would show a different headline to ?h=2. I've called each item of content in a slot a Card. The card value of a slot it retrieved through the following method.

<?php
$page = new Page($data);
$headline = $page->get('headline');
?>
<h1><?=$headline?></h1>

However, a slot may contain another slot, where for example, a headline could have the content "Welcome back, {username} from {location}" and each tag name is processed and existing Slot objects with those names are injected into its parent object.

  1. Should I instead try and create a reference to those slots instead of injecting them?
  2. Would this consume a lot of memory if someone tried to nest them multiple times, or if there are many nested slots or if a slot holds 10 000 possible cards?
  3. Below is the main class Page, where the slots are injected in the constructor. Followed by the Slot class, should this be done at instantiation and/or should I have a method for updating the configuration?

If you need more scope, the repository is currently on GitHub

Page.php

<?php

namespace Kamereon;

use Symfony\Component\HttpFoundation\Request;

/**
 *  The base for a new dynamic landing page. Each dynamic placeholder is called a slot
 *  where a slot will hold many cards for one to be displayed depending on a set of
 *  given parameters.
 *
 *  @author Adam
 */
class Page
{
    /**
     *  The Symfony HttpFoundation Request object.
     */
    protected $request;

    /**
     *  Raw configuration data.
     */
    protected $config = array();

    /**
     *  Collection of Slot objects.
     */
    protected $slots  = array();

    /**
     *  Loads the config data and creates new Slot instances.
     *
     *  @param array $config
     */
    public function __construct(array $config)
    {
        $this->request = Request::createFromGlobals();
        $this->config  = $config;

        // created new instances for each slot
        foreach ($config['slots'] as $slotName => $slotData) {
            $this->slots[$slotName] = new Slot($slotName, $slotData);
        }

        // inject nested slots
        foreach ($config['slots'] as $slotName => $slotData) {
            if (isset($slotData['nestedWith']) && count($slotData['nestedWith']) > 0) {
                foreach ($slotData['nestedWith'] as $nestedSlotName) {
                    $this->slots[$slotName]->addNestedSlot($this->slots[$nestedSlotName]);
                }
            }
        }
    }

    /**
     *  Get the configuration array.
     *
     *  @return array
     */
    public function getConfig()
    {
        return $this->config;
    }

    /**
     *  Get a Slot object from the slot collection by its key.
     *
     *  @param string $slot
     *  @return Slot
     */
    public function getSlot($slot)
    {
        return $this->slots[$slot];
    }

    /**
     *  Get the card value for a slot. 
     *
     *  @param  string $slotName
     *  @param  string $default
     *  @return string
     */
    public function get($slotName, $default = '0')
    {
        $slot = $this->slots[$slotName];

        try {
            $card = $slot->getCard($this->request->get($slot->getKeyBind(), $default));
        } catch (\Exception $e){
            $card = '';
        }

        if ($slot->getHasNestedSlots()) {

            foreach ($slot->getNestedSlots() as $nestedSlot) {
                try {
                    $nestedCards[$nestedSlot->getName()] = $nestedSlot->getCard(
                        $this->request->get($nestedSlot->getKeyBind(), $default)
                    );
                } catch (\Exception $e){
                    $nestedCards[$nestedSlot->getName()] = '';
                }
            }

            foreach ($nestedCards as $cardName => $cardValue) {
                $card = str_replace(
                    sprintf('{%s}', $cardName),
                    $cardValue,
                    $card
                );
            }
        }

        return $card;
    }

    /**
     *  Override the request instance by injecting your own.
     *
     *  @param Request $request
     */
    public function setRequest(Request $request)
    {
        $this->request = $request;
    }

    /**
     *  Get the request instance.
     *
     *  @return Request
     */
    public function getRequest()
    {
        return $this->request;
    }
}

Slot.php

<?php

namespace Kamereon;

/**
 *  A placeholder for variable content on a page, which a value will be assigned
 *  to it as a Card instance
 *
 *  @author Adam 
 */
class Slot
{
    /**
     *  The name of the slot
     */
    protected $name;

    /**
     *  The key name that is bound to the slot
     *  A key can be shared with another slot
     */
    protected $keyBind;

    /**
     *  An array of the names of nested slots
     */
    protected $nestedSlotNames = array();

    /**
     *  The collection array of nested Slot objects
     */
    protected $nestedSlots = array();

    /**
     *  A list of cards for each one will be displayed on the page
     */
    protected $cards = array();

    /**
     *  Create new slot with name, key binding and its cards
     *  and if the slot has nested slots, assign only the names of
     *  those slots.
     *
     *  @param string $name
     *  @param array  $data
     */
    public function __construct($name, array $data)
    {
        $this->name    = $name;
        $this->keyBind = $data['keyBind'];
        $this->cards   = $data['cards'];

        if (isset($data['nestedWith'])) {
            $this->nestedSlotNames = $data['nestedWith'];
        }
    }

    /**
     *  Get the name of the slot
     *
     *  @return string
     */
    public function getName()
    {
        return $this->name;
    }

    /**
     *  Add a slot to the nested slots collection
     *
     *  @param Slot $slot
     */
    public function addNestedSlot(Slot $slot)
    {
        $this->nestedSlots[$slot->getName()] = $slot;
    }

    /**
     *  Get all nested slots
     *
     *  @return array
     */
    public function getNestedSlots()
    {
        return $this->nestedSlots;
    }

    /**
     *  Get specific nested slot
     *
     *  @return Slot
     */
    public function getNestedSlotByName($name)
    {
        return $this->nestedSlots[$name];
    }

    /**
     *  Get a value of a card by its index / array key.
     *  Throws an InvalidArgumentException if the key does not exist.
     *
     *  @return string
     */
    public function getCard($index)
    {
        if (array_key_exists($index, $this->cards)) {
            return $this->cards[$index];
        }
        throw new \InvalidArgumentException(sprintf(
            'Card with index "%s" for slot "%s" does not exist', $index, $this->name));
    }

    /**
     *  Get the binded key
     *
     *  @return string
     */
    public function getKeyBind()
    {
        return $this->keyBind;
    }

    /**
     *  Check if a slot contains other slots nested within
     *
     *  @return boolean
     */
    public function getHasNestedSlots()
    {
        return count($this->nestedSlots) > 0;
    }
}
share|improve this question

2 Answers 2

up vote 1 down vote accepted

I'm going to try hard to answer your questions. I'm a little confused by how you're doing this. It seems like you're trying to generate a complicated request from $_GET and instead of just loading up what you need, you've created a complicated structure that loads things up and then unpacks them later. I think you could more easily load things up into an array instead of nesting things, but it's always hard to figure out what people are really trying to do. If you can provide a more clear problem case, you might get better responses.

My attempt at answers:
1) I would inject the request, first of all, instead of using a static method. Next, I wouldn't have that new operator in the constructor. I would inject an array of Slots and just assign that: $this->slots = $slots; __construct(Request $request, $slots){}

2) When you start talking about 10,000 cards, I start to wonder more about what you're doing. It doesn't seem particularly intensive, but you may end up having to up your memory allocation if you're using large objects. I'll defer to someone else on this. I usually wait until I hit upper limits before I think about resources. Not my area.

3) You actually aren't injecting it properly. Also, you're building your request object twice. I wouldn't use a set method. I would stick with dependency injection. I don't really know what you mean by configuration, but anything your class needs should be passed into your constructor, not set later. It's less confusing that way and it guarantees you have everything you need for your object.

share|improve this answer
    
Thanks Stephane, that's pretty helpful. Good point about the request object, but I'd wanted a default object to be instantiated instead of the user explicitly injecting one. How to do this correctly? Also I originally had an array instead of nested objects and it worked well, but found it much harder to worker nested values, and could only be nested once. Plus it was more difficult to write tests for. Just realised I'm missing the configuration array from my post, I'll add it to the question, with a more realistic sample. –  Adam-E Dec 14 '12 at 9:57
    
I've just made a commit that allows an optional Request object to be passed, or default to creating one from PHP globals. –  Adam-E Dec 15 '12 at 17:50

I must criticize your tests. They are not part of your question here, but available in your github repo.

Your SlotTest does not test slots. It does test the page. Why?

class SlotTest extends \PHPUnit_Framework_TestCase
{
    protected $page;
    protected function setUp()
    {
        require __DIR__.'/../fixtures/config.php';
        $this->page = new Page($kamereon);
    }

Reading the tests, I do not really see how a single slot is to be used. I only see how multiple slots inside a page are used. Also, I only see tests for several GET methods. If I want to verify that the results that are coming out are correct, I somehow have to read the config fixture file - a secondary source of information.

This situation actually is bad. I want to be able to read the tests, and actually CHANGE some input values in a certain test method to see whether or not it changes the output and breaks the test. If I cannot see the input, I cannot play with the values.

One final improvement: If you include files, they can actually return a value. You do not need to define a global variable that will transfer the config values.

// don't use this
$kamereon = array(...);

// use this
return array(...);

// Fetching values then is like this:
$kamereon = include('config.php');
share|improve this answer
    
Thanks for the suggestions. I'm relatively new to writing tests, so if you know of good resources for unit testing best practices that be great. SlotTest and PageTest could be combined into one file, and still cover both Slot and Page, but were you expecting that SlotTest cover the usage of a Slot by itself? –  Adam-E Dec 15 '12 at 23:39
    
Also, good idea regarding return the included file. I've done it like this because the configuration could potentially be in xml, yaml, json, from a database, etc –  Adam-E Dec 15 '12 at 23:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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