Controller: Sign up action clean up

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)

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:

• Redefine what AccountController should do, and refactor the code to fit that model.

• Fix the formatting of the code in AccountController.

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

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

StackExchange Code Review Q#56915, answer score: 3