The short answer to both of your questions:
Is this a valid controller?
Yes, it is. It seems to have a clear, single responsability. All a controller does is process the request data (ie pour it into objects), and pass it on to the model layer (services), and handle what the services return in a way so that the view can be created. You do all this, just fine (Apart from one thing, which I'll get to in a minute).
A controller action shouldn't contain a lot of code, yours don't. It seems to me to be clean code, and a correct application of the MVC principles.
Any points I could improve on (suggestions)?
There are always things to improve, but in this case, there aren't a lot, and -apart from one issue- they're all minor things.
First things first: all of your
use statements have the leading
\ namespace separator. This is not recommended. Quoting the official docs:
Note that for namespaced names (fully qualified namespace names containing namespace separator, such as Foo\Bar as opposed to global names that do not, such as FooBar), the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.
So simply get rid of them. Rule of thumb: only use the leading
\ if you want to
use a global class or constant:
use \PDO;//<-- global, needs \
Your code is not compatible with PHP versions prior to 5.4. That, in itself, isn't that big of a deal (people should be upgrading to 5.4, or 5.5), but still. An expression like this one:
'csrfToken' => $this->httpRequest->findParameter($this->antiCsrfService->getToken()['name']),
Can be easily rewritten as:
$token = $this->antiCsrfService->getToken();
'csrfToken' => $this->httpRequest->findParameter($token['name']),
Giving better backwards compatibility. This is of course completely optional. Take it or leave it, really.
Another suggestion from me to you, with the same goal (of supporting older versions) would be to use the old-school
array() notation, instead of the newer
. But again: this is optional, and I for one don't mind code that urges people to upgrade their PHP version, instead of having to write code to support dated versions. So again: no real need to change this.
On to more valuable suggestions
There are a couple of optional, yet more important things I'd change if I were you:
Use doc-blocks, if you don't already. Any half-decent IDE will use the doc-blocks for code-completion. This tells the users (people working with your code) what arguments any given method expects, what the possible exceptions are, what the method does and what it returns. This can (and if used correctly will) cut down on development time considerably. It'll also make debugging easier. IDE's highlighting a method call with invalid arguments, for instance. For example:
isValid methods in some frameworks return a boolean value, others return the token value if it's valid, or
null if it isn't. I'd like to know what this
isTokenValid method returns, without having to open the class definition itself. In which case, the method should have a matching doc-block along these lines:
* Check if the token is valid
* @param string $token?
* @return bool
* @throws InvalidArgumentException
public function isTokenValid($token)
//check $token type, assess if it is/can be a token, if not
//throw an exception, else return true if valid, false if not
Change the doc-block if you're returning the token itself, of course.
Class properties, like the ones you have, should have a doc-block, telling the user (and the IDE) what the value of each property will be:
* @var Model\Application\AntiCsrfService;
private $antiCsrfService = null;
This will make the IDE's job of autocompleting calls a lot easier, and again: it makes your code a lot easier to use for those who aren't familiar with its internal workings.
You do seem to be doing a lot of programming by array. As I've stated earlier, IMHO, the job of a controller is to pre-process the request data by pouring it into objects, and pass those objects on to the services. You do the same thing, only: you're pouring all data into arrays. Which in turn means the service layer possibly contains one to many methods with a signature like:
public function processData(array $data)
This makes your code more error-prone than it needs to be. If I want to process a form, which contains an email address, I'd use a data model, with a
setEmail method. This method would call
filter_var($email, \FILTER_VALIDATE_EMAIL) and throw an exception if the email address is invalid.
Seeing as you're using a more recent version of PHP, you could create a trait with this email method, and share this method between all models that might contain an email address (I'm thinking: comment submissions, user registration, user updates, ...)
This programming by array has another downside, which has already manifested itself in the code you posted here. One-lining excessively:
'lastName' => $user['lastName'] = trim($this->httpRequest->findParameter('lastName')),
I get that assigning things to a variable, only to then assign it to a newly constructed array is tedious, but one-liners like this harm readability, make code more error-prone, results in ridiculously long statements and can be easily avoided through use of setters. Have the
setLastName method call
trim. Doing the trimming in the controller will inevitably lead to repetition.
The other upside of using data models is that you can test the setters. Testing what each statement in a controller does is possible, too, but it's more cumbersome than writing a simple unit-test for a data model, which only requires you to test the getters and setters (and possibly bulk methods).
The same advice as ever:
Try to adhere to the coding standards as much as you can. I'm not perfect in this respect either, but take your constructor for instance: at 250 chars in length, it does not conform to the PSR-2 guidelines. Its length is more than twice that of the soft limit (120).
The FIG standard also recommends you split an argument list like yours over several lines (see PSR-2, 4.4 Method Arguments for details).
The biggest issue:
AFAIK, there is only 1 thing I'd really consider to be a flaw. Well, more of a urgent TODO:
This method, at least in the code you show here seems to only take one argument. Most of the existing request objects, used in the major frameworks allow you to pass 2 arguments: the first being the name of the argument, and the second being the default return value in case the parameter was not found:
Http\HttpRequest::findParameter($name, $default = null);
Which allows you to call:
$userName = $this->httpRequest->findParamter('name');
if ($userName === null)
//handle incomplete data
But in case of optional values, to set a default value:
$comments = $this->httpRequest->findParameter('comments', '');
//carry on, if none given, it'll contain an empty string.
Your code is fine, it really is. Just try to reduce the use of arrays somewhat, ensure that the backwards compatibility is not an issue for you, and consider adding some more generic features (like the default argument for the
Other than that
$this->view->success() seems to me to be an unfortunate method-name, but seeing as you said in your question that you've chosen to call views in your controller to keep it simple, I'm assuming this code is a one-off. Slight deviations like this are, to an extent, acceptable.