Controller: Sign up action clean up

Question

I don't know if it's just me, but I feel like my sign up action method has gotten a bit messy.

I think that perhaps instead of setting error messages in a controller, I should set error codes which I can intercept in the View and assign the actual text message to it in there. Because I think it's classified as UI logic. I'm not sure how I would do that code-wise.

I commented the code to make it easily understandable and would like to get some input on cleaning it up and also regarding the following aspects:

• Readability
• Efficiency
• Usability

Any other suggestions for improvement will be appreciated as well.

Including the parent controller class, in case someone wants to see it:

namespace Controllers;
use \vHttp\Request;
use \vHttp\Cookie;
use \vHttp\Session;
use \vHttp\Response;
use \Models\UserService;
abstract class Controller
{
 protected $request;
 protected $cookie;
 protected $session;
 protected $response;
 protected $userService;
 protected $view;
 public function __construct(Request $request, Cookie $cookie, Session $session, Response $response, UserService $userService)
 {
 $this->request = $request;
 $this->cookie = $cookie;
 $this->session = $session;
 $this->response = $response;
 $this->userService = $userService;
 $class = explode('\\', get_class($this));
 $class = '\Views\\' . $class[count($class) - 1];
 $this->view = new $class();
 }
}

AccountController

namespace Controllers;
use \Libraries\Validator\Validator;
use \Libraries\Validator\Rules\MaxChars;
use \Libraries\Validator\Rules\Alpha;
use \Libraries\Validator\Rules\Email;
use \Libraries\Validator\Rules\Match;
use \Libraries\Validator\Rules\MinChars;
use \Libraries\CryptoCharGen;
class Account extends Controller
{
 public function signUp()
 {
 // if the session is new, create a token and tie it to the session just once.
 // (only for forms)
 if (!$this->cookie->getParameter('sid')) {
 $this->session->setParameter('csrfToken', CryptoCharGen::alnum());
 }
 // variable for if-statement readability
 $csrfToken = array(
 'post' => $this->request->getParameter('csrfToken'),
 'session' => $this->session->getParameter('csrfToken')
 );
 // on form submit where token is also valid
 if ($this->request->getMethod() === 'POST' && $csrfToken['post'] === $csrfToken['session']) {
 // user's form sign up data
 $signee = array(
 'firstname' => trim($this->request->getParameter('firstname')),
 'lastname' => trim($this->request->getParameter('lastname')),
 'email' => trim($this->request->getParameter('email')),
 'cfmEmail' => trim($this->request->getParameter('cfmEmail')),
 'password' => $this->request->getParameter('password'),
 'terms' => $this->request->getParameter('terms')
 );
 // field validation rules
 $rules = array(
 'firstname' => [new MaxChars(35), new Alpha(true)],
 'lastname' => [new MaxChars(35), new Alpha(true)],
 'email' => [new Email()],
 'cfmEmail' => [new Email(), new Match($signee['email'])],
 'password' => [new MinChars(6)],
 'terms' => []
 );
 // set custom error response
 $rules['firstname'][0]->setError('First name too long');
 $rules['firstname'][1]->setError('First name, only alpha letters allowed');
 $rules['lastname'] [0]->setError('Last name too long');
 $rules['lastname'] [1]->setError('Last name, only alpha letters allowed');
 $rules['email'] [0]->setError('Email invalid');
 $rules['cfmEmail'] [0]->setError('Email invalid');
 $rules['cfmEmail'] [1]->setError('Email doesn\'t match');
 $rules['password'] [0]->setError('Password must be at least 6 characters');
 // instantiate and initialize v, with custom error response for required fields
 $v = new Validator();
 $v->setRule('firstname', $rules['firstname'], 'First name required');
 $v->setRule('lastname', $rules['lastname'], 'Last name required');
 $v->setRule('email', $rules['email'], 'Email required');
 $v->setRule('cfmEmail', $rules['cfmEmail'], 'Confirm email required');
 $v->setRule('password', $rules['password'], 'Password required');
 $v->setRule('terms', $rules['terms'], 'Must agree to the terms');
 // validate all signee's inputs
 $v->validate('firstname', $signee['firstname']);
 $v->validate('lastname', $signee['lastname']);
 $v->validate('email', $signee['email']);
 $v->validate('cfmEmail', $signee['cfmEmail']);
 $v->validate('password', $signee['password']);
 $v->validate('terms', $signee['terms']);
 // if signee's data passed the validation, safe to TRY and register the user
 if (!$v->getErrors()) {
 try {
 $this->userService->signUp( 
 //this method will throw an exception if anything exceptional happens
 $signee['firstname'],
 $signee['lastname'],
 $signee['email'],
 $signee['password']
 );
 // If no exceptions thrown, sign up was successful, so now POST-REDIRECT-GET.
 // using session to persist signee's email that's to be used upon showing their email
 // on the successful signup view at the end of the POST-REDIRECT-GET.
 $this->session->setParameter('signeeEmail', $signee['email']); 
 // refresh csrf token
 $this->session->setParameter('csrfToken', CryptoCharGen::alnum());
 // exit script and redirect to self
 $this->response->redirect(); 
 } catch (\PDOException $e) {
 // catch PDO unique email constraint and set validator error for email field
 $v->setError('email', 'Email already taken');
 }
 }
 // if php reaches this piece of code, means validation did not pass, and POST-REDIRECT-GET did not happen.
 // dispatch view with the error responses, and raw (untrimmed) form input data to repopulate the fields and form token.
 return $this->view->signUp(
 array(
 'csrfToken' => $this->session->getParameter('csrfToken'), 
 'formErrors' => $v->getErrors(),
 'inputVals' => array(
 'firstname' => $this->request->getParameter('firstname'),
 'lastname' => $this->request->getParameter('lastname'),
 'email' => $this->request->getParameter('email'),
 'cfmEmail' => $this->request->getParameter('cfmEmail'),
 'password' => $this->request->getParameter('password'),
 'terms' => $this->request->getParameter('terms')
 )
 )
 );
 } elseif ($data['signeeEmail'] = $this->session->getParameter('signeeEmail')) {
 // end of self redirection of POST-REDIRECT-GET
 $this->session->unsetParameter('signeeEmail');
 // dispatch successful sign up view
 return $this->view->signUp(['signeeEmail' => $data['signeeEmail']], true);
 }
 // dispatch clean GET view with no errors or anything, just form token when visiting /signup
 return $this->view->signUp(['csrfToken' => $this->session->getParameter('csrfToken')]);
 }
}

• Madara's Ghost: At first glance: It isn't the Controller's responsibility to create the View. The controller should, at most, return the View's name, and have the bootstrap process instantiate it with a proper DIC.
• Kid Diamond: Then how do I pass data from the controller to the corresponding view?
• Madara's Ghost: Controller updates the Model layer with new state, View queries the Model layer for information. Controller and View are not aware of each other.
• Kid Diamond: Okay, but form validation is not altering anything in the model layer, so everything my form validator returns will be lost because the view can't get that data back from the model layer.
• Madara's Ghost: Why would your view care about something the form validator returns? If there were validation errors, then the FormErrors object (or whatever) should be bound to the Model somehow.

Answer 1

Before I get started, I just want to say I really like your questions. I've been able to see the progression you've made as you move along throughout your new framework project, and it's quite fascinating!

I would like to say that this piece of code is, like you said, hard to read. What can we do about that? I see three viable solutions:

1. Redefine what AccountController should do, and refactor the code to fit that model.
2. Fix the formatting of the code in AccountController.
3. Redefine the classes you depend on, to make your life easier down the scope and road.

You mention efficiency, but it's hard to tell how to optimize this without first seeing some times. How can you figure out what's the fastest part and what's the slowest part without first knowing their speeds?

---

Let's start with our solution part 1. We need to redefine what should be done. Right now, it seems you're unsure of what the class should be doing.

There are many versions of MVC, MVP, and other design patterns out there. I'm not going to say that I am correct, however I'm going to share my opinion (oh no...).

I think that your PHP framework should follow a pattern such as:

MVC strategy. Separate Model and View

Source

To some, this may not be a "true MVC" pattern, but you have to ask yourself, is a PHP framework really the place for a "true MVC" pattern? Of course not, it wouldn't make any sense.

The Wikipedia article for MVC tells us that the model should handle business logic. However, for PHP, I think it makes more sense for the controller to handle this, and leave the model up to figuring out a data request.

I think Symphony's explanation of their PHP controller is relevant:

A controller is a PHP function you create that takes information from the HTTP request and constructs and returns an HTTP response. The response could be an HTML page, an XML document, a serialized JSON array, an image, a redirect, a 404 error or anything else you can dream up. The controller contains whatever arbitrary logic your application needs to render the content of a page.

We are also given this explanation:

The goal of a controller is always the same: create and return a Response object. Along the way, it might read information from the request, load a database resource, send an email, or set information on the user's session. But in all cases, the controller will eventually return the Response object that will be delivered back to the client.

Just because there are patterns such as MVC out there, doesn't mean they have to be followed exactly, you can bend them to your preference.

If you need to see some actual code, here's an example of a Mediator. It might help you see a way to structure your framework.

Now, you may be wondering how to fit in the data source. If you're using a database, I highly suggest taking a look at Doctrine. Study it and see how it gets things done.

But how is this relevant to AccountController?

I find that all of this can help you figure out what the controllers should be doing, and how they should do it. In my opinion, this class has too much going on in it, and it'd depending on too many different resources. With the correct pattern applied to this, I think you could reduce the dependencies and clear up your class.

---

In your class, there's a lot of visual repetition. I think this is the largest issue in terms of readability. Repeating words over and over can trick the reader's eyes.

Seeing a dozen $this->request is fine. However, as soon as you through in a $this->response, it get's quite confusing. The reader becomes accustomed to seeing the one word, and then a curve ball gets thrown at them and the word is now different. How else can I describe it besides being confusing! I think reading a Wikipedia page about duplicate code might help.

The repetition of largely identical code sections can conceal how they differ from one another, and therefore, what the specific purpose of each code section is.

I definitely think that if the code was repeated less, the brain would take more time to process what it's reading, and therefore it would be easier to parse the code. I think MrLore's answer tried to get something at that.

See if you can alter some of the classes you're using, so that the calling code is less repetitive for each instance. I know that each field requires unique handling, but I also know that there are ways to circumvent that repetition.

---

As I just said, perhaps the best way to clean up your code, would be to go up a level, and clean up the classes you depend on.

I count 28 lines for the validation of 6 fields. How can the validation class be changed so that larger quantities of data can be used against it?

I think a lot of your troubles are in the classes this object depends on. I highly suggest you refactor those first.

Now besides that, let's look at this:

Request $request, Cookie $cookie, Session $session, Response $response, UserService $userService

This is something I believe to be a leading cause to the confusion in your class. Don't tell anyone "this is how it has to be". Let's revisit my first point, you can make this how ever it needs to be! Just because you think you need the five dependencies, doesn't mean they all have to be jammed in here.

Think about what you can change to reduce this list. That doesn't necessarily mean combining object to make two clean ones, one messy one.

---

I think you know what needs to be done. Look at how data, logic, and presentation need to interact together. Look at how you can simplify the code and make things easier to read. Look at how the classes you call are being built.

Now, besides all of that stuff, there's a couple other little things I'd like to point out:

• If the user of the framework decides to have a folder in Views, using get_class($this) won't work out very well. As of now, I believe one level of directories is supported.
• Instead of $class[count($class) - 1], you could just use end ($class). Don't worry, it's not as hack-y as it seems!
• $signee seems weird, why not registerer?

• Kid Diamond: Thank you for your valuable review. I've been really trying to master this entire MVC pattern stuff, and make me a solid framework. Because I know once I got it completely down packed, creating web applications with it would be so much quicker.
• Madara's Ghost: @KidDiamond TBH, creating a wall-to-wall framework will almost surely lead to failure. I've given up on trying a full fledged framework, in favor of smaller reusable modules (The Router, The Autoloader, The Form object, etc). Requirements change, a full framework, however modular will not be able to adapt to all the changes and all new projects you may have. (Although for a learning experience, it's great, keep going :D)
• Kid Diamond: @MadaraUchiha I'm not trying to create a full blown framework, I'm actually just trying to make the core components that will be reusable throughout any application I develop. Like Router, Session, Cookie, Request, as you mentioned. And indeed, I have taken this route and my knowledge has increased tremendously so far and the journey is fun.
• Madara's Ghost: @KidDiamond Excellent, just noting :)

Answer 2

The first thing that jumps out is all the Strings you have which are reused throughout, those need to be removed and ideally put into constants so you only have to change them in one place if you ever need to rename them:

const FIRST_NAME = 'firstname';
const LAST_NAME = 'lastname';
const EMAIL = 'email';
const CONFIRM_EMAIL = 'cfmEmail';
const PASSWORD = 'password';
const TERMS = 'terms';

You do a lot of mapping from parameters to arrays and vice versa, which you could simplify by storing an array of the parameter names and whether or not you want them trimmed, for example:

/**
 * Map of field names to whether they should be trimmed by default.
 */
private static $BIND_PARAMS = array(self::FIRST_NAME => true, self::LAST_NAME => true,
 self::EMAIL => true, self::CONFIRM_EMAIL => true,
 self::PASSWORD => false, self::TERMS => false);
private function getBoundParams($useDefaultTrim = true)
{
 $params = array();
 foreach(self::$BIND_PARAMS as $parameter => $trim)
 {
 $params[$parameter] = $this->request->getParameter($parameter);
 if($useDefaultTrim && $trim)
 {
 $params[$parameter] = trim($params[$parameter]);
 }
 }
 return $params;
}

Would allow you to replace this:

$signee = array(
 'firstname' => trim($this->request->getParameter('firstname')),
 'lastname' => trim($this->request->getParameter('lastname')),
 'email' => trim($this->request->getParameter('email')),
 'cfmEmail' => trim($this->request->getParameter('cfmEmail')),
 'password' => $this->request->getParameter('password'),
 'terms' => $this->request->getParameter('terms')
);

With this:

$signee = $this->getBoundParams();

And this:

return $this->view->signUp(
 array(
 'csrfToken' => $this->session->getParameter('csrfToken'), 
 'formErrors' => $v->getErrors(),
 'inputVals' => array(
 'firstname' => $this->request->getParameter('firstname'),
 'lastname' => $this->request->getParameter('lastname'),
 'email' => $this->request->getParameter('email'),
 'cfmEmail' => $this->request->getParameter('cfmEmail'),
 'password' => $this->request->getParameter('password'),
 'terms' => $this->request->getParameter('terms')
 )
 )
);

Could be reduced to this:

return $this->view->signUp(
 array(
 'csrfToken' => $this->session->getParameter('csrfToken'),
 'formErrors' => $v->getErrors(),
 'inputVals' => $this->getBoundParams(false)
 )
);

• Madara's Ghost: -1 for constants and statics. Immutable global state is still global state. No please.
• MrLore: @MadaraUchiha Actually, I was intending them to be in the AccountController class, as anyone who knows PHP should have been able to work out by the fact they were referenced as self::LAST_NAME in the code.
• Madara's Ghost: All statics are globals. It doesn't matter which class they belong to. Same as class constants.