Today while creating a simple controller for a Silex application, I noticed a number of action methods had almost identical code:
public function someAction()
{
$id = $this->request->attributes->get('id')
if($id)
{
$data = getMyDataFromDatabaseWithThisID($id);
//do something with data
$this->app->json($data, 200);
}
else
{
//do error stuff
}
}
public function someOtherAction()
{
$id = $this->request->attributes->get('id')
if($id)
{
$data = getMyDataFromDatabaseWithThisID($id);
//do something else with data
$this->app->json($data, 200);
}
else
{
//do error stuff
}
}
The bones of each method is pretty much exactly the same. I figured if I have 3 or 4 methods following the same pattern, I should probably abstract out some of that code.
My first instinct was to have a big executeAction($action) method, but I do not know of a way of passing a variable to the called controller action from a Silex route. Second, I figured replacing lots of smallish methods with one massive one was asking for trouble.
My next thought was to create an executeAction(Closure $action) method that was called by the existing action methods passing a closure with the task needed.
So an action method such as updateAction() could pass a closure to executeAction() telling it to update the desired resource:
public function updateAction
{
$this->executeAction(function($resource, $request) {
$resource->doSomethingToUpdateItFromInfoInRequest($request->get('data'));
});
}
public function executeAction(Closure $action)
{
$id = $this->request->attributes->get('id')
if($id)
{
$data = getMyDataFromDatabaseWithThisID($id);
//execute closure action
$action($data, $this->request);
$this->app->json($data, 200);
}
else
{
//do error stuff
}
}
On the face of it this looks quite clean. It is also flexible in that it is up to each individual method whether or not executeAction is used or not. The issue I have with this idea is that I need to make sure anything done in the closure is properly reflected outside the closure (i.e. are variables/objects being updated correctly etc). It also doesn't feel like the right way to do it. Though this could be due to closures in PHP being fairly new to me (before playing with Silex I never used them in my PHP code).
The third option is to reduce some of the common parts to methods of their own - such as
public function getResourceById($id)
{
//do stuff
return $resource;
}
public function errorStuff($code, $message)
{
//do stuff
}
//then in action method
public function updateAction()
{
$data = $this->getResourceById($this->request->get('id');
if($data)
{
//do stuff
}
else
{
$this->errorStuff(1001, 'uh oh!');
}
}
The method looks smaller, but the same problem exists - I will have several methods that look virtually identical and will need to be updated if something changes (method names etc).
So, given I am using Silex and a Silex Controller class which of these approaches, if any, is the better choice for avoiding code repetition? Can anyone suggest a different solution entirely?