4
\$\begingroup\$

I have some questions on how I can improve this "action" (method) in "controller":

  1. My template has a navbar with dynamic content (if the user is logged in a special button appears, his name appears in the navbar among other custumizações) also the integration of modes is dynamic based on whether the user is logged in or not, this insert in my controller calls some methods that will check if the user are logged and enter the data into the template.

  2. I am inserting code that search the database (Query Builder) directly into the action.

  3. I am using a comment line like "//--------------------------" to separate concepts and operations. Is it correct?

  4. I'm using one full template for every action (e.g.: One full template for action (method) add of Products Controller) (Note: most of pages (add, edit, delete, index) use different .js and .css files).

Controller:

<?php
namespace App\Controller;

use App\Controller\AppController;
use Cake\Event\Event;
use Cake\ORM\TableRegistry;

class StoresController extends AppController
{
     public function miniMap()
    {
        $setting = [
            'fields' => ['id', 'banner_description', 'path_banner', 'url_redirect'],
            'conditions' => ['banner_type_id' => 2],
            'limit' => 1
        ];
        $fullBanners = TableRegistry::get('Banners')
            ->find('all', $setting)->hydrate(false)->toArray();
        $this->set('fullBanners', $fullBanners);

        //-------------------------------------------------------------------------

        $setting = [
            'fields' => ['id', 'banner_description', 'path_banner', 'url_redirect'],
            'conditions' => ['banner_type_id' => 1],
            'limit' => 3
        ];
        $smallBanners = TableRegistry::get('Banners')
            ->find('all', $setting)->hydrate(false)->toArray();
        $this->set('smallBanners', $smallBanners);

        //-------------------------------------------------------------------------

        $this->set('userId', $this->Auth->user('id'));

        //-------------------------------------------------------------------------

        $this->set('username', $this->Auth->user('username'));
    }
}

Template:

<?php    
    $this->layout = false;
?>
<!DOCTYPE html>
<html>
    <head>
        <?= $this->Html->charset() ?>
        <?= $this->Html->meta('viewport','width=device-width, initial-scale=1.0') ?>
        <?= $this->Html->meta('title',$pageTitle) ?>
        <?= $this->Html->meta('favicon.ico','/cart.png', ['type' => 'icon']) ?>
        <?= $this->Html->meta('keywords','') ?>
        <?= $this->Html->meta('description','') ?>
        <?= $this->Html->meta('robots','index,follow') ?>

        <?= $this->Html->css('library/datepicker/css/datepicker.css') ?>
        <?= $this->Html->css('library/bxslider-4-4.1.2/jquery.bxslider.css') ?>
        <?= $this->Shrink->css(['styles/style.css', 'styles/menu-plugin.css']) ?>

        <?= $this->Html->script('library/bxslider-4-4.1.2/jquery.bxslider.min.js',['defer' => true]) ?>
        <?= $this->Html->script('actions/main.js',['defer' => true]) ?>

        <?= $this->Shrink->fetch('css') ?>
    </head>
    <body>
        <?= $this->element('Navbar/navbar_main') ?>
        <div class="wrapper">
            <div class="container">
                <div class="row">
                    <?= $this->element('Body/categories') ?>
                    <?= $this->element('Body/stores') ?>
                </div>
            </div>
        </div>
        <?= $this->element('Footer/footer_information') ?>
        <?php if ($userId == false): ?>
            <?= $this->element('Modal/create_account_modal') ?>
            <?= $this->element('Modal/login_modal') ?>
        <?php else: ?>
            <?= $this->element('Modal/logout_modal') ?>
        <?php endif; ?>
    </body>
</html>
\$\endgroup\$
3
  • \$\begingroup\$ There's an odd habit of putting the whole layout in the view file, and not using layouts at all - why are you doing that (Definitely not considered a best practice)? \$\endgroup\$
    – AD7six
    Commented Dec 4, 2015 at 9:50
  • \$\begingroup\$ @AD7six Full template is because the css and js is different between other templates, you can my code? I can put a bounty on it. \$\endgroup\$
    – Ricardo
    Commented Dec 4, 2015 at 16:03
  • \$\begingroup\$ @Ricardo there's no need for a bounty - see how to link to asset files - and the use of the block option, there should always be a layout file - even if it's one-per-template IMO. \$\endgroup\$
    – AD7six
    Commented Dec 4, 2015 at 17:09

1 Answer 1

Reset to default
1
\$\begingroup\$

There are numerous things you can do to make the code easier to maintain.

Put code in your models

The minimap function calls find on the banners table twice - with config that doesn't change. It'd make things a lot cleaner to do this:

class BannerTable extends Table 
{
    public function full()
    {
        $setting = [
            'fields' => ['id', 'banner_description', 'path_banner', 'url_redirect'],
            'conditions' => ['banner_type_id' => 2],
            'limit' => 1
        ];
        return $this
            ->find('all', $setting)->hydrate(false)->toArray();
    }

    public function small()
    {
        $setting = [
            'fields' => ['id', 'banner_description', 'path_banner', 'url_redirect'],
            'conditions' => ['banner_type_id' => 1],
            'limit' => 3
        ];
        return $this
            ->find('all', $setting)->hydrate(false)->toArray();
    }
}

As should be apparent written like this, the settings arrays are near duplicates - you can easily consolidate the two methods into one, or change into finder methods as you see fit.

The controller code then becomes:

public function miniMap()
{
    $this->loadModel('Banners');
    $fullBanners = $this->Banners->full();
    $smallBanners = $this->Banners->small();
    $userId = $this->Auth->user('id');
    $username = $this->Auth->user('username');

    $this->set(compact('fullBanners', 'smallBanners', 'userId', 'username'));
}

Which is much more concise. This also does away with any need to delimit the code with comment lines (I wouldn't recommend that, if you feel it's necessary put each delimited block of code in a separate method).

Note that the auth component stores data in the session, which is accessible in the view. Consider reading the current user data directly out of the session rather than passing variables around which contain information that's duplicated elsewhere.

Use caching

There's no need to get the banner info from the db on every request, you can wrap that in a cache call:

use Cake\Cache\Cache;
...

public function miniMap()
{
    list($fullBanners, $smallBanners) = Cache::remember('banners', function () {
        $this->loadModel('Banners');
        $fullBanners = $this->Banners->full();
        $smallBanners = $this->Banners->small();
        return [$fullBanners, $smallBanners];
    });

    $userId = $this->Auth->user('id');
    $username = $this->Auth->user('username');
    $this->set(compact('fullBanners', 'smallBanners', 'userId', 'username'));
}

Template structure

The template is clean and easy to read but it is not a good habit to put the layout in the view file. Especially since the reason given is this:

Full template is because the css and js is different between other templates

A view file should look more like this:

<?php

$this->Html->css('library/datepicker/css/datepicker.css', ['block' => true]);
...
?>
<ul>
    <?php foreach($examples as $example): ?>
    <li><?= .. whatever code to output the content ?> </li>
    <?php endforeach; ?>        
</ul>

With a layout that looks more like this:

<html>
    <head>
        <?= $this->fetch('meta');?>
        <?= $this->fetch('css');?>
    </head>
    <body>
        <?= $this->fetch('content'); ?>
        <?= $this->fetch('script'); ?>
    </body>
</html>

By making use of the block option, the same layout file can be used even though the css/js/whatever in each template is different. If the layout structure differs - That's a good reason to use a different layout file via:

$this->layout = 'different';

in the template file.

Miscellaneous comments

  1. Don't use classes you aren't using - Event is not used
  2. Load models using loadModel - Using the table registry of course works, but loading models in controllers is normally done with loadModel - it is more efficient.
  3. The template makes no reference to the variables being set in the method miniMap - fullBanners, smallBanners and username are unused - if they aren't used don't set them.
  4. Provide a complete example - The controller code and template don't relate to each other - it's hard to put either into an appropriate context as the code in the question, whilst complete enough to comment on, isn't complete enough to see how it's used.
\$\endgroup\$
3
  • \$\begingroup\$ Re list - thanks for pointing that out, corrected. It's obvious the variables are used elsewhere but that isn't in the question. Your new templates look fine, though the scripts should not be in the head. In another action - new question in a new question please. That said: the user id is in the data, but not the cache key - you're going to be using the data for one user, as the favorites for all users; you need to account for clearing the cache when favorites data changes. You seem to be using the banner variables in multiple actions - put the code to set that in the app controller. \$\endgroup\$
    – AD7six
    Commented Dec 7, 2015 at 8:50
  • 1
    \$\begingroup\$ I don't speak Portuguese =). You're welcome to cross-reference this answer and update your posts there. \$\endgroup\$
    – AD7six
    Commented Dec 14, 2015 at 10:56
  • \$\begingroup\$ AD7six I put two questions on stackoverflow that I think you can answer: stackoverflow.com/questions/34263687/… and stackoverflow.com/questions/34263566/… \$\endgroup\$
    – Ricardo
    Commented Dec 14, 2015 at 12:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.