15
\$\begingroup\$

I've asked a couple of questions here before regarding my controller, but I think I've finally gotten to understand controllers completely, but I'll never be sure unless I get my code reviewed.

I've moved all my application logic into service classes and placed those inside the application layer of my model layer. I'm using my controller solely to control the application layer based on the request and calling the appropriate view. The parent controller has no logic other then setting the dependencies.

  • Is this a valid controller?

  • Any points I could improve on (suggestions)?

    Readability (I'm always concerned about proper naming), usability, efficiency.

Note: I do know that controllers originally aren't supposed to call views, but I'm handling it this way just for the sake of making things not too complicated.

namespace Controller;

use \View\SignUpView;
use \Http\HttpRequest;
use \Model\Application\AntiCsrfService;
use \Model\Application\RegistrationService;
use \Model\Application\AuthenticationService;
use \Model\Application\FormValidationService;

class SignUpController extends Controller
{
    private $antiCsrfService;
    private $formValidationService;
    private $registrationService;

    public function __construct(HttpRequest $httpRequest, AuthenticationService $authenticationService, AntiCsrfService $antiCsrfService, RegistrationService $registrationService, FormValidationService $formValidationService, SignUpView $signUpView)
    {
        parent::__construct($httpRequest, $authenticationService, $signUpView);

        $this->antiCsrfService       = $antiCsrfService;
        $this->formValidationService = $formValidationService;
        $this->registrationService   = $registrationService;

        if ($this->authenticationService->findLoggedInUser()) {
            $this->view->redirectToHome();
        }
    }

    public function index()
    {
        // ↓ End of Post/Redirect/Get after successful User registration
        if ($user = $this->registrationService->findFlashUser()) {
            return $this->view->success([
                'user' => [
                    'firstName' => (string) $user->getFirstName(),
                    'lastName'  => (string) $user->getLastName(),
                    'email'     => (string) $user->getEmail()
                ]
            ]);
        }

        return $this->view->index([
            'form' => [
                'csrfToken' => $this->antiCsrfService->getToken()
            ]
        ]);
    }

    public function submit()
    {
        $formInputs = [
            'csrfToken'         => $this->httpRequest->findParameter($this->antiCsrfService->getToken()['name']),
            'firstName'         => $user['firstName'] = trim($this->httpRequest->findParameter('firstName')),
            'lastName'          => $user['lastName']  = trim($this->httpRequest->findParameter('lastName')),
            'email'             => $user['email']     = trim($this->httpRequest->findParameter('email')),
            'confirmationEmail' =>                      trim($this->httpRequest->findParameter('confirmationEmail')),
            'password'          => $user['password']  =      $this->httpRequest->findParameter('password'),
            'terms'             =>                           $this->httpRequest->findParameter('terms')
        ];

        if ($this->antiCsrfService->isTokenValid($formInputs['csrfToken'])) {
            $formInputs = array_slice($formInputs, 1);

            if (!$this->formValidationService->isSignUpValid($formInputs)) {
                return $this->view->index([
                    'form' => [
                        'values'    => $formInputs,
                        'errors'    => $this->formValidationService->findErrorsOf('signUp'),
                        'csrfToken' => $this->antiCsrfService->getToken()
                    ]
                ]);
            }

            $this->registrationService->registerUser($user['firstName'], $user['lastName'], $user['email'], $user['password']);
        }

        return $this->view->redirectToSelf();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I think that your question hasn't received a lot of answers because your code is good. I could only think of two minor points: 1. findLoggedInUser would better be named isUserLoggedIn or isLoggedIn (you are not really searching for any logged in user, but asking if this specific user is logged in); 2. Constructor arguments: I would pull $signUpView in third place, so it aligns with how the arguments are used. Apart from that, your formValidationService seems a bit odd. Is it one giant class that has methods for all kinds of forms (the method calls would suggest this)? \$\endgroup\$ – tim Sep 1 '14 at 17:56
  • \$\begingroup\$ @tim The reason I named it findLoggedInUser is because it returns a User entity if found. The formValidationService is indeed pretty much a potential giant class that contains the technical validation rules for every form. But it doesn't violate the SRP or anything. I just don't know any better solution. :( I probably should extract the repeating validation callables into reusable private methods. And indeed too bad that I haven't gotten any useful answers. I think answers that point out the good things are as valuable as ones that point out the bad things when doing code reviewing. \$\endgroup\$ – Kid Diamond Sep 1 '14 at 18:18
  • \$\begingroup\$ @KidDiamond: I agree with tim. I didn't review this code, seeing as I didn't think there was a lot to actually say about the code itself. It's one of the better pieces of code on this site, and most reviewers tend to focus on inefficiencies or stylistic issues. Your code is good, apart from a few things here and there. A review pointing those minor things out to you might put people off, thinking this site is filled with style Nazi's. Saying what is good about this code is just one sentence: It's a correct application of the MVC pattern, Anyway: i've given it a go, and posted my review \$\endgroup\$ – Elias Van Ootegem Sep 2 '14 at 10:03
  • \$\begingroup\$ PS: I did upvote your code, on the basis that posting a well written piece of code merits some rep, if only to make up for the rep you lost awarding the bounty... \$\endgroup\$ – Elias Van Ootegem Sep 2 '14 at 10:05
2
+50
\$\begingroup\$

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.


Minor improvements/suggestions

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:

namespace MyNS;
use Base\Core\Dependency;
use \PDO;//<-- global, needs \

Minor things:
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:

$this->antiCsrfService->isTokenValid($formInputs['csrfToken']);

Some 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:

Http\HttpRequest::findParameter

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.

Recap:

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 findParameter method).
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.

\$\endgroup\$
  • \$\begingroup\$ I almost thought my question was a lost cause here. You have pointed out some things I haven't thought of. Luckily, you are just in time before the bounty expiration. :) \$\endgroup\$ – Kid Diamond Sep 2 '14 at 10:05
  • 1
    \$\begingroup\$ @KidDiamond: believe it or not, I did not notice the bounty, until after I posted this review :D. Still, I hope this review makes up for my comment (the name conflict thing, which I deleted just now). Hope it proves useful in some way \$\endgroup\$ – Elias Van Ootegem Sep 2 '14 at 10:07
  • \$\begingroup\$ Do you have any remarks on me having a FormValidationService class that contains is[FormName]Valid(args); (as seen in my code) methods for every form of my application? It contains a set of frequently used validation callables extracted into reusable private methods that I use to build each is[FormName]Valid(); method. Or would this better be an entire new question in itself? :P \$\endgroup\$ – Kid Diamond Sep 2 '14 at 10:45
  • 1
    \$\begingroup\$ @KidDiamond: I think that topic should be discussed in a different question all together. But I will say this: if you want to write a form validation service, I'd consider writing form classes, and validator classes which can be linked to individual input types. Each form would then consist of form elements, which can be attached to an individual validator. Creating a generic validateForm method would suffice, then, because you'd simply get all validators, and apply them to each element, setting errors along the way \$\endgroup\$ – Elias Van Ootegem Sep 2 '14 at 11:31
-1
\$\begingroup\$

Line 22 could be reduced to:

if ($this->authService->findLoggedInUser()) $this->view->redirectToHome;

This is an arguable point, but I don't think you need to typecast the variables to strings. It should instead be the responsibility of the object User to return the correct type by default. E.g return (string) $this->firstName within your User model.

'firstName' => (string) $user->getFirstName(),
'lastName'  => (string) $user->getLastName(),
'email'     => (string) $user->getEmail()

Line 54, $form doesn't really hit the nail on the head for naming. Perhaps inputs, form_values.

$form = [
    'firstName'         => trim($this->httpRequest->getParameter('firstName')),
    'lastName'          => trim($this->httpRequest->getParameter('lastName')),
    'email'             => trim($this->httpRequest->getParameter('email')),
    'confirmationEmail' => trim($this->httpRequest->getParameter('confirmationEmail')),
    'password'          => $this->httpRequest->getParameter('password'),
    'terms'             => $this->httpRequest->getParameter('terms')
];

Line 73. Why are we redirecting to ourselves? More redirects, more problems. Can't you just do return $this->index()?

return $this->view->redirectToSelf();

I might also recommend checking to ensure that the method is a post. And also doing your password hashing etc. from your controller level if you plan on hiding that.

There's also no validation that any of the values exist? I see that there are "form errors". Typically your controller handles the rules behind form validation IMO. Perhaps another service actually validates, but the controller should be the one setting the rules for the interop between the view and the model.

Here are a couple of examples from popular frameworks to illustrate

http://laravel.com/docs/validation

https://ellislab.com/codeigniter/user-guide/libraries/form_validation.html

\$\endgroup\$
  • 2
    \$\begingroup\$ It's also an arguable point that properties should be exposed how they are set. You set a property of type Email, expose it as such. I don't like if-statements without brackets, I try to stay consistent. We're redirecting to self because it's part of a Post / Redirect / Get. Password hashing is not the controllers responsibility. I'm doing that in the service class, same goes for validation. Skinny controllers, fat models. What you actually suggested is the way CodeIgniter is doing it by having fat controllers with business logic. I've found CI to be a bad example of the MVC pattern. \$\endgroup\$ – Kid Diamond Aug 27 '14 at 11:47
  • \$\begingroup\$ Laravel, Magento, Angular, Backbone. They too all follow the idea that the controller takes the role of user input validation rulemaker. a service(model) in and of itself is both dumb to the exact intent of the user when they made the request and the resting place of that data. A controller's action offers the greatest clarity into the function in which it is suppose to serve. Models are CRUD operators and the data should only become their responsibility after it has been augmented by more finite responsibility holders. IMO \$\endgroup\$ – ajameswolf Aug 27 '14 at 12:06
  • \$\begingroup\$ The Model Layer should contain application, domain, and infrastructure logic where CRUD operators reside in the Infrastructure Layer. The Model Layer is not only for CRUD operators, but a lot more. A controller is supposed to control the Application Layer. If you take a controller out of your application and notice missing application, domain, and/or infrastructure logic, that means you had logic in your controller that was supposed to be in the Model Layer. \$\endgroup\$ – Kid Diamond Aug 27 '14 at 13:16

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.