Is my LoginView class valid?

Question

I know the Model, Controller and View's purpose, but I have never really found a good concrete class example of a View class.

Usually I see people having some small render() method being called from the Controller, or their Controller holds all the View logic. So I tried my best to create a View class that seemed proper and somewhat clean to me in regards to the MVC pattern.

I've found a useful image that shows how the data flow is supposed to be and how the View sits in it.

• Is this a valid View class?

• Anything I could improve on / suggestions?

  Readability, usability and / or efficiency.

---

LoginView.php

namespace View;

use View\View;

class LoginView extends View
{
    private $isUserLoggedIn;
    private $isFormTokenValid;

    public function index()
    {
        // Logged in Users have no reason to view this page.
        if ($this->isUserLoggedIn) {
            $this->httpResponse->redirect('/');
        }

        // Assume csrf attack, refresh self for a fresh View.
        if ($this->isFormTokenValid === false) {
            $this->httpResponse->redirect();
        }

        // Header title.
        $this->templateData['title'] = 'Log In - Site Name';

        // Header stylesheets.
        $this->templateData['styleSheets'] = [
            '/stylesheets/login.css'
        ];

        // Header javascripts.
        $this->templateData['javaScripts'] = [
            '//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js',
            '/javascripts/login-form-handler.min.js'
        ];

        // Form input error messages.
        $formErrorMessages = [
            'email' => [
                'NotEmpty' => 'Enter your email address.',
                'Email'    => 'You did not enter a valid email address.'
            ],
            'password' => [
                'NotEmpty' => 'Enter your password.'
            ]
        ];

        // Replace form error codes with corresponding error messages.
        if (isset($this->templateData['form']['errors'])) {
            $this->templateData['form']['errors'] = $this->replaceArrayValuesByKey($this->templateData['form']['errors'], $formErrorMessages);
        }

        // If not set by User input, check 'remember' checkbox by default.
        if (!isset($this->templateData['form']['values']['remember'])) {
            $this->templateData['form']['values']['remember'] = true;
        }

        $this->renderTemplate();
    }

    public function setIsUserLoggedIn($boolean)
    {
        $this->isUserLoggedIn = $boolean;
    }

    public function setIsFormTokenValid($boolean)
    {
        $this->isFormTokenValid = $boolean;
    }

    public function setFormValueOf($name, $value)
    {
        $this->templateData['form']['values'][$name] = $value;
    }

    public function setFormErrorCodeOf($name, $code)
    {
        $this->templateData['form']['errors'][$name] = $code;
    }

    public function setHasLoginFailed($boolean)
    {
        $this->templateData['hasLoginFailed'] = $boolean;
    }
}

View.php

namespace View;

use Http\HttpResponse;

abstract class View
{
    protected $httpResponse;
    protected $templatePaths;
    protected $templateData;

    public function __construct(
        HttpResponse $httpResponse,
        $headerTemplatePath = null,
        $bodyTemplatePath   = null,
        $footerTemplatePath = null
    ) {
        $this->httpResponse = $httpResponse;

        if ($headerTemplatePath) {
            $this->templatePaths['header'] = $headerTemplatePath;
        }

        if ($bodyTemplatePath) {
            $this->templatePaths['body'] = $bodyTemplatePath;
        }

        if ($footerTemplatePath) {
            $this->templatePaths['footer'] = $footerTemplatePath;
        }
    }

    public function replaceArrayValuesByKey(array $target, array $source)
    {
        foreach ($target as $k => $v) {
           for ($i = 0, $c = count($v); $i < $c; ++$i) {
              if (isset($source[$k][$v[$i]])) {
                   $target[$k][$i] = $source[$k][$v[$i]];
              }
           }
        }

        return $target;
    }

    public function renderTemplate()
    {
        extract($this->templateData);

        if (isset($this->templatePaths['header'])) {
            require_once $this->templatePaths['header'];
        }

        if (isset($this->templatePaths['body'])) {
            ob_start([$this, 'autoIndent']);

            require_once $this->templatePaths['body'];

            ob_end_flush();
        }

        if (isset($this->templatePaths['footer'])) {
            require_once $this->templatePaths['footer'];
        }
    }

    private function autoIndent($buffer)
    {
        $content = '';
        $lines   = explode(PHP_EOL, $buffer);

        // TODO: calculate indentation levels of header template

        foreach ($lines as $line) {
            $content .= str_repeat(' ', 12) . $line . PHP_EOL;
        }

        return $content;
    }
}

You can view the LoginController class that controls this View here, if interested.

Answer 1

As @Pinoniq mentioned in the comments, a view shouldn't be responsible for redirect users via business rules. The responsibility of a view should be solely to convert a PHP template into HTML and to capture that output into a variable to be used as you see fit.

Based on that criteria alone, the redirect violates that rule. That sort of stuff should all be happening at the controller level. As soon as you require data to be passed into a view, your controller will need to check whether the user is logged in or not before it can fetch their corresponding user data so it makes sense to have it there from that perspective alone.

Not to mention from a testing perspective, it's much easier to test that re-direct at the controller level than it is to test it in the view. This also removes the dependency on HttpResponse which means that you don't need to mock a response just to test your view creation logic now which is also a big win. Assuming you're writing tests for all of this logic ;)

Finally, I'd highly recommend taking a look at Twig or some other PHP compatible template engine. It seems to me like you're getting a bit over zealous in defining a lot of your page header and body blocks programatically whereas you'd be much better off in taking this same approach, but with template inheritance instead, in a template engine.

P.S. I don't think initializing an array with = [ ]; is valid PHP for your $formErrorMsg.