FEATURE: Raise The Code Quality

[dfsmania]

I believe we should raise the code quality, for this, we can follow the next 2 tasks:

1) Use Scrutinizer as a guide for this, we can start attacking the Hot Spots:

2) Start checking that new Pull Requests does not downgrade the code quality.

I'm planning to give a try to the AdminLte class as a start point, and use it as an example here.

[REJack]

That's exactly why I've created the issue #566 to increase the code quality 😄. I work already on the console commands.

[dfsmania]

@REJack Yeah, I know, that is one step, however AdminLte class is now fully cover by tests but still bad rated as F by complexity or some duplicated code.

[REJack]

Yeah the complexity is everytime one of the shittiest point on code quality, also I hate scrutinizer it doesn't show which function is too complex and kills the score. I use the phpunit's coverage-html to check this 😄. We can also add some private/protected functions to reduce the duplicate code lines.

[REJack]

I've tested some changes and increased the AdminLte class rating to B, you can checkout it here. Also the overall test coverage is raised to 68% (+31%) but I'm not finished the tests for the artisan commands 😄.

May we split some classes out and reduce the overall complexity a bit.

My latest scrutinizer inspection based on this commit https://github.com/REJack/Laravel-AdminLTE/commit/6978552f2c3f74e2c87a327ec7333bb2f89dc70a.

[dfsmania]

@REJack Ok, very nice, I was also working on the AdminLTE class complexity, but with a different approach. I have decided to create a new set of helper methods:

src/Menu/Helpers/MenuItemHelper.php:

<?php

namespace JeroenNoten\LaravelAdminLte\Menu\Helpers;

class MenuItemHelper
{
    /**
     * Check if a menu item is a header.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isHeader($item)
    {
        return is_string($item) || isset($item['header']);
    }

    /**
     * Check if a menu item is a link.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isLink($item)
    {
        return isset($item['text']) && (isset($item['url']) || isset($item['route']));
    }

    /**
     * Check if a menu item is a search bar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSearchBar($item)
    {
        return isset($item['text']) && isset($item['search']) && $item['search'];
    }

    /**
     * Check if a menu item is a submenu.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSubmenu($item)
    {
        return isset($item['text']) && isset($item['submenu']);
    }

    /**
     * Check if a menu item is valid for the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isValidNavbarItem($item)
    {
        if (self::isHeader($item)) {
            return false;
        }

        return self::isLink($item) ||
               self::isSearchBar($item) ||
               self::isSubmenu($item);
    }

    /**
     * Check if a menu item is valid for the sidebar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isValidSidebarItem($item)
    {
        return self::isHeader($item) ||
               self::isLink($item) ||
               self::isSearchBar($item) ||
               self::isSubmenu($item);
    }

    /**
     * Check if a menu item belongs to the left section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarLeftItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav']) && $item['topnav'];
    }

    /**
     * Check if a menu item belongs to the right section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarRightItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav_right']) && $item['topnav_right'];
    }

    /**
     * Check if a menu item belongs to the user menu section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarUserItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav_user']) && $item['topnav_user'];
    }

    /**
     * Check if a menu item belongs to the sidebar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSidebarItem($item)
    {
        if (! self::isValidSidebarItem($item)) {
            return false;
        }

        return ! self::isNavbarLeftItem($item) &&
               ! self::isNavbarRightItem($item) &&
               ! self::isNavbarUserItem($item);
    }
}

I was planing to use they in the AdminLte filter functions:

   /**
     * Filter method for sidebar menu items.
     */
    private function sidebarFilter($item)
    {
        return MenuItemHelper::isSidebarItem($item);
    }

    /**
     * Filter method for navbar top left menu items.
     */
    private function navbarLeftFilter($item)
    {
        if (config('adminlte.layout_topnav') && MenuItemHelper::isSidebarItem($item)) {
            return MenuItemHelper::isValidNavbarItem($item);
        }

        return MenuItemHelper::isNavbarLeftItem($item);
    }

    /**
     * Filter method for navbar top right menu items.
     */
    private function navbarRightFilter($item)
    {
        return MenuItemHelper::isNavbarRightItem($item);
    }

    /**
     * Filter method for navbar dropdown user menu items.
     */
    private function navbarUserMenuFilter($item)
    {
        return MenuItemHelper::isNavbarUserItem($item);
    }

And also in some of the blade files, where we check if an item is header, submenu, search form, etc. This reduces the complexity, with the exception of the getBodyClasses() that I will attack later.

[REJack]

That’s a nice approach too, lets see which brings the best impact 😄

[dfsmania]

Sure, but I will only play with the AdminLte class... The artisan commands are all yours :smile:

[dfsmania]

@REJack You can see an overview of my approach on the Shidersz@raise_admilte_class_quality branch. I have created two new helpers files (MenuItemHelper.php and LayoutHelper.php) inside the src/Helpers folder.

MenuItemHelper Class

<?php

namespace JeroenNoten\LaravelAdminLte\Helpers;

class MenuItemHelper
{
    /**
     * Check if a menu item is a header.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isHeader($item)
    {
        return is_string($item) || isset($item['header']);
    }

    /**
     * Check if a menu item is a link.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isLink($item)
    {
        return isset($item['text']) && (isset($item['url']) || isset($item['route']));
    }

    /**
     * Check if a menu item is a search bar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSearchBar($item)
    {
        return isset($item['text']) && isset($item['search']) && $item['search'];
    }

    /**
     * Check if a menu item is a submenu.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSubmenu($item)
    {
        return isset($item['text']) && isset($item['submenu']);
    }

    /**
     * Check if a menu item is valid for the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isValidNavbarItem($item)
    {
        if (self::isHeader($item)) {
            return false;
        }

        return self::isLink($item) ||
               self::isSearchBar($item) ||
               self::isSubmenu($item);
    }

    /**
     * Check if a menu item is valid for the sidebar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isValidSidebarItem($item)
    {
        return self::isHeader($item) ||
               self::isLink($item) ||
               self::isSearchBar($item) ||
               self::isSubmenu($item);
    }

    /**
     * Check if a menu item belongs to the left section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarLeftItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav']) && $item['topnav'];
    }

    /**
     * Check if a menu item belongs to the right section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarRightItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav_right']) && $item['topnav_right'];
    }

    /**
     * Check if a menu item belongs to the user menu section of the navbar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isNavbarUserItem($item)
    {
        if (! self::isValidNavbarItem($item)) {
            return false;
        }

        return isset($item['topnav_user']) && $item['topnav_user'];
    }

    /**
     * Check if a menu item belongs to the sidebar.
     *
     * @param mixed $item
     * @return boolean
     */
    public static function isSidebarItem($item)
    {
        if (! self::isValidSidebarItem($item)) {
            return false;
        }

        return ! self::isNavbarLeftItem($item) &&
               ! self::isNavbarRightItem($item) &&
               ! self::isNavbarUserItem($item);
    }
}

LayoutHelper Class

<?php

namespace JeroenNoten\LaravelAdminLte\Helpers;

use Illuminate\Support\Facades\View;

class LayoutHelper
{
    /**
     * Set of tokens related to screen sizes.
     *
     * @var array
     */
    protected static $screenSizes = ['xs', 'sm', 'md', 'lg', 'xl'];

    /**
     * Check if layout topnav is enabled.
     *
     * @return boolean
     */
    public static function isLayoutTopnavEnabled()
    {
        return config('adminlte.layout_topnav') || View::getSection('layout_topnav');
    }

    /**
     * Check if layout boxed is enabled.
     *
     * @return boolean
     */
    public static function isLayoutBoxedEnabled()
    {
        return config('adminlte.layout_boxed') || View::getSection('layout_boxed');
    }

    /**
     * Make and return the set of classes related to the body tag.
     *
     * @return array
     */
    public static function makeBodyClasses()
    {
        $classes = [];

        $classes = array_merge($classes, self::makeLayoutClasses());
        $classes = array_merge($classes, self::makeSidebarClasses());
        $classes = array_merge($classes, self::makeRightSidebarClasses());
        $classes = array_merge($classes, self::makeCustomBodyClasses());

        return $classes;
    }

    /**
     * Make and return the set of data attributes related to the body tag.
     *
     * @return array
     */
    public static function makeBodyData()
    {
        $data = [];

        // Add data related to the "sidebar_scrollbar_theme" configuration.

        $sb_theme_cfg = config('adminlte.sidebar_scrollbar_theme', 'os-theme-light');

        if ($sb_theme_cfg != 'os-theme-light') {
            $data[] = 'data-scrollbar-theme='.$sb_theme_cfg;
        }

        // Add data related to the "sidebar_scrollbar_auto_hide" configuration.

        $sb_auto_hide = config('adminlte.sidebar_scrollbar_auto_hide', 'l');

        if ($sb_auto_hide != 'l') {
            $data[] = 'data-scrollbar-auto-hide='.$sb_auto_hide;
        }

        return $data;
    }

    /**
     * Make and return the set of classes related to the layout configuration.
     *
     * @return array
     */
    private static function makeLayoutClasses()
    {
        $classes = [];

        // Add classes related to the "layout_topnav" configuration.

        if (self::isLayoutTopnavEnabled()) {
            $classes[] = 'layout-top-nav';
        }

        // Add classes related to the "layout_boxed" configuration.

        if (self::isLayoutBoxedEnabled()) {
            $classes[] = 'layout-boxed';
        }

        // Add classes related to fixed sidebar layout configuration. The fixed
        // sidebar is not compatible with layout topnav.

        if (! self::isLayoutTopnavEnabled() && config('adminlte.layout_fixed_sidebar')) {
            $classes[] = 'layout-fixed';
        }

        // Add classes related to fixed navbar/footer configuration. The fixed
        // navbar/footer is not compatible with layout boxed.

        if (! self::isLayoutBoxedEnabled()) {
            $classes = array_merge($classes, self::makeFixedResponsiveClasses('navbar'));
            $classes = array_merge($classes, self::makeFixedResponsiveClasses('footer'));
        }

        return $classes;
    }

    /**
     * Make the set of classes related to a fixed responsive configuration.
     *
     * @param string $section
     * @return array
     */
    private static function makeFixedResponsiveClasses($section)
    {
        $classes = [];
        $cfg = config('adminlte.layout_fixed_'.$section);

        if ($cfg === true) {
            $classes[] = 'layout-'.$section.'-fixed';
        } elseif (is_array($cfg)) {
            foreach ($cfg as $size => $enabled) {
                if (in_array($size, self::$screenSizes)) {
                    $size = $size == 'xs' ? '' : '-'.$size;
                    $classes[] = $enabled == true ?
                        'layout'.$size.'-'.$section.'-fixed' :
                        'layout'.$size.'-'.$section.'-not-fixed';
                }
            }
        }

        return $classes;
    }

    /**
     * Make the set of classes related to the left sidebar configuration.
     *
     * @return array
     */
    private static function makeSidebarClasses()
    {
        $classes = [];

        // Add classes related to the "sidebar_mini" configuration.

        if (config('adminlte.sidebar_mini', true) === true) {
            $classes[] = 'sidebar-mini';
        } elseif (config('adminlte.sidebar_mini', true) == 'md') {
            $classes[] = 'sidebar-mini sidebar-mini-md';
        }

        // Add classes related to the "sidebar_collapse" configuration.

        if (config('adminlte.sidebar_collapse') || View::getSection('sidebar_collapse')) {
            $classes[] = 'sidebar-collapse';
        }

        return $classes;
    }

    /**
     * Make the set of classes related to the right sidebar configuration.
     *
     * @return array
     */
    private static function makeRightSidebarClasses()
    {
        $classes = [];

        // Add classes related to the "right_sidebar" configuration.

        if (config('adminlte.right_sidebar') && config('adminlte.right_sidebar_push')) {
            $classes[] = 'control-sidebar-push';
        }

        return $classes;
    }

    /**
     * Make the set of classes related to custom body classes configuration.
     *
     * @return array
     */
    private static function makeCustomBodyClasses()
    {
        $classes = [];
        $cfg = config('adminlte.classes_body', '');

        if (is_string($cfg) && $cfg) {
            $classes[] = $cfg;
        }

        return $classes;
    }
}

These helpers are mainly used by the AdminLte class, but the idea is to use some of the available methods on the blade files. For example, I'm expecting to rewrite resources/views/partials/navbar/menu-item.blade.php:

@if (isset($item['search']) && $item['search'])

    {{-- Search form --}}
    @include('adminlte::partials.navbar.menu-item-search-form')

@elseif (isset($item['submenu']))

    {{-- Dropdown menu --}}
    @include('adminlte::partials.navbar.menu-item-dropdown-menu')

@else

    {{-- Link --}}
    @include('adminlte::partials.navbar.menu-item-link')

@endif

like this:

@if (MenuItemHelper::isSearchbar($item))

    {{-- Search form --}}
    @include('adminlte::partials.navbar.menu-item-search-form')

@elseif (MenuItemHelper::isSubmenu($item))

    {{-- Dropdown menu --}}
    @include('adminlte::partials.navbar.menu-item-dropdown-menu')

@elseif (MenuItemHelper::isLink($item))

    {{-- Link --}}
    @include('adminlte::partials.navbar.menu-item-link')

@endif

But currently I don't know how to get MenuItemHelper class imported to be used on the blade files (any feedback about this is welcome).

This is the latest scrutinizer inspection related to the mentioned branch, but still have to do some adjustments.

[REJack]

That looks nice 😄 may this stackoverflow thread helps with the class import inside the blade files. I would try option 4 Using a class and service injection.

[resslinger]

I will make a new release, but I wait for the test improvments so no stress :)

[dfsmania]

@REJack Nicely, I have just tried the option 4 and it worked. So, do you say I should make a PR today for those improvements, or we should go with the ones you have made?

[REJack]

I would create a PR for your changes they look good 😃 and have a better rating than mine.

[dfsmania]

@REJack, @lacodimizer the PR #610 has been created and should be tested now.

[resslinger]

@Shidersz great work!

[resslinger]

Can we close this issue?

[REJack]

I would say yes, but Idk what @Shidersz would say 😄

[dfsmania]

Actually, this is more a Feature than an Issue. There are still some points to attack. The only reason I will keep this is just to remenber there are work to do, but feel free to close if you want.

[REJack]

We can use my issue #566 to keep an eye on the changes, its better than have 2 issue open 😄

[dfsmania]

@REJack Sure, just edit that one and add a mention to improve methods rated as F by scrutinizer. I close this one.

[REJack]

I've update my issue 😄