ShortListController::performAction() - Code Metrics - Inspection of "Refactoring of url actions on controller" - salted-herring/silverstripe-shortlist - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Branch — master (a3e8b1)

by Simon

created 2016-04-18 07:15 UTC

ShortListController::performAction() C

↳ Parent: ShortListController

Complexity

Conditions	12
Paths	13

Size

Total Lines	55
Code Lines	37

Duplication

Lines	18
Ratio	32.73 %

Importance

Changes	1
Bugs	0	Features	0

Metric	Value
c	1
b	0
f	0
dl	18
loc	55
rs	6.8009
cc	12
eloc	37
nc	13
nop	1

How to fix Long Method Complexity

<?php

class ShortListController extends Page_Controller
{
    private static $allowed_actions = array(
        'renderList',
        'performAction'
    );

    private static $url_handlers = array(
        'add'       => 'performAction',
        'remove'    => 'performAction',
        '$URL!'     => 'renderList',
    );

    private static $extensions = array(
        'ShortListPaginationExtension'
    );

    public function init()
    {
        parent::init();

        Session::start();

        if ($this->request->getVar('page')) {
            $this->currentPage = $this->request->getVar('page');
        }
    }

    /**
     * When landing on the homepage, if there is a shortlist for the current
     * user, redirect to the correct URL. Otherwise, 404.
     * */
    public function index($request)
    {
        if (($shortlist = $this->getSessionShortList())) {
            return $this->redirect(Config::inst()->get('ShortList', 'URLSegment') . $shortlist->URL);
        } else {
            /*
if (!ShortList::isBrowser()) {
                return $this->httpError(404);
            }
*/

            $shortlist = $this->getSessionShortList();

            if (!$shortlist || !$shortlist->exists()) {
                $shortlist = new ShortList();
                $shortlist->write();
            }
        }

        // render with empty template.
        return $this->renderWith(array('Page', 'ShortList_empty'));
    }

    /**
     * Get the absolute URL of this controller.
     * */
    public function Link($action = null)
    {
        $shortlist = $this->getSessionShortList();
        $url = Config::inst()->get('ShortList', 'URLSegment');

        if ($shortlist) {
            $url .= $shortlist->URL;
        }

        return $url;
    }

    public function renderList($request)
    {
        $shortlist = DataObject::get_one('ShortList', $filter = array('URL' => $request->param('URL')));

        if (is_null(session_id()) ||
            !$request->param('URL') ||
            !$shortlist ||
            !$shortlist->exists()
        ) {
            return $this->httpError(404);
        }

        $link = false;
        $count = 0;

        if ($shortlist && $shortlist->exists()) {
            $link = $shortlist->Link();
            $count = $shortlist->ShortListItems()->Count();
        }

        return $this->customise(array(
            'ShortlistURL' => $link,
            'ShortlistCount' => $count
        ))->renderWith(
            array('ShortList', 'Page')
        );
    }

    public function performAction($request)
    {
        if (is_null(session_id()) ||
            !$request->getVar('id') ||
            !$request->getVar('type') ||
            !$request->getVar('s') ||
            $request->getVar('s') != session_id()
        ) {
            return $this->httpError(404);
        }

        $matches = array();
        preg_match('/remove|add/', $request->getURL(), $matches);

        switch ($matches[0]) {
            case 'add':

                $action = new AddToshortlistAction();
                $status = $action->performAction(
                    $shortlist = $this->getSessionShortList(),
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
                    $ID = $request->getVar('id'),
                    $type = $request->getVar('type'),
                    $session = $request->getVar('s')
                );
                break;
            case 'remove':

                $action = new RemoveFromshortlistAction();
                $status = $action->performAction(
                    $shortlist = $this->getSessionShortList(),
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
                    $ID = $request->getVar('id'),
                    $type = $request->getVar('type'),
                    $session = $request->getVar('s')
                );
                break;
        }

        if ($request->isAjax()) {
            $shortlist = $this->getSessionShortList();
            $url = false;

            if ($shortlist && $shortlist->exists()) {
                $url = $shortlist->Link();
            }

            return json_encode(array(
                'count' => $this->shortListCount($session),
function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}
                'url' => $url
            ));
        }

        if (array_key_exists('output', $request->getVars())) {
            return $status;
function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}
        }

        return $this->redirectBack();
    }


    /**
     * Get the number of items in the current short list.
     *
     * @param session The session to check & find a shortlist for.
     * @return mixed false if no session exists - else the number of items in the shortlist.
     * */
    public function shortListCount($session = false)
    {
        if (is_null(session_id()) || !$session || $session != session_id()) {
            return false;
        }

        $shortlist = $this->getSessionShortList();

        if (!$shortlist || !$shortlist->exists()) {
            return 0;
        }

        return $shortlist->Items()->count();
    }

    private function getSessionShortList()
    {
        return DataObject::get_one('ShortList', $filter = array('SessionID' => session_id()), $cache = false);
    }

    public static function getShortListSession()
    {
        return DataObject::get_one('ShortList', $filter = array('SessionID' => session_id()));
    }
}


1		<?php
2
3		class ShortListController extends Page_Controller
4		{
5		private static $allowed_actions = array(
6		'renderList',
7		'performAction'
8		);
9
10		private static $url_handlers = array(
11		'add' => 'performAction',
12		'remove' => 'performAction',
13		'$URL!' => 'renderList',
14		);
15
16		private static $extensions = array(
17		'ShortListPaginationExtension'
18		);
19
20		public function init()
21		{
22		parent::init();
23
24		Session::start();
25
26		if ($this->request->getVar('page')) {
27		$this->currentPage = $this->request->getVar('page');
28		}
29		}
30
31		/**
32		* When landing on the homepage, if there is a shortlist for the current
33		* user, redirect to the correct URL. Otherwise, 404.
34		* */
35		public function index($request)
36		{
37		if (($shortlist = $this->getSessionShortList())) {
38		return $this->redirect(Config::inst()->get('ShortList', 'URLSegment') . $shortlist->URL);
39		} else {
40		/*
41		if (!ShortList::isBrowser()) {
42		return $this->httpError(404);
43		}
44		*/
45
46		$shortlist = $this->getSessionShortList();
47
48		if (!$shortlist \|\| !$shortlist->exists()) {
49		$shortlist = new ShortList();
50		$shortlist->write();
51		}
52		}
53
54		// render with empty template.
55		return $this->renderWith(array('Page', 'ShortList_empty'));
56		}
57
58		/**
59		* Get the absolute URL of this controller.
60		* */
61		public function Link($action = null)
62		{
63		$shortlist = $this->getSessionShortList();
64		$url = Config::inst()->get('ShortList', 'URLSegment');
65
66		if ($shortlist) {
67		$url .= $shortlist->URL;
68		}
69
70		return $url;
71		}
72
73		public function renderList($request)
74		{
75		$shortlist = DataObject::get_one('ShortList', $filter = array('URL' => $request->param('URL')));
76
77		if (is_null(session_id()) \|\|
78		!$request->param('URL') \|\|
79		!$shortlist \|\|
80		!$shortlist->exists()
81		) {
82		return $this->httpError(404);
83		}
84
85		$link = false;
86		$count = 0;
87
88		if ($shortlist && $shortlist->exists()) {
89		$link = $shortlist->Link();
90		$count = $shortlist->ShortListItems()->Count();
91		}
92
93		return $this->customise(array(
94		'ShortlistURL' => $link,
95		'ShortlistCount' => $count
96		))->renderWith(
97		array('ShortList', 'Page')
98		);
99		}
100
101		public function performAction($request)
102		{
103		if (is_null(session_id()) \|\|
104		!$request->getVar('id') \|\|
105		!$request->getVar('type') \|\|
106		!$request->getVar('s') \|\|
107		$request->getVar('s') != session_id()
108		) {
109		return $this->httpError(404);
110		}
111
112		$matches = array();
113		preg_match('/remove\|add/', $request->getURL(), $matches);
114
115		switch ($matches[0]) {
116	View Code Duplication	case 'add':
		0 ignored issues – show Duplication introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
117		$action = new AddToshortlistAction();
118		$status = $action->performAction(
119		$shortlist = $this->getSessionShortList(),
		0 ignored issues – show Documentation introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report `$shortlist = $this->getSessionShortList()` is of type `object<DataObject>`, but the function expects a `object<ShortList>\|null`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
120		$ID = $request->getVar('id'),
121		$type = $request->getVar('type'),
122		$session = $request->getVar('s')
123		);
124		break;
125	View Code Duplication	case 'remove':
		0 ignored issues – show Duplication introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
126		$action = new RemoveFromshortlistAction();
127		$status = $action->performAction(
128		$shortlist = $this->getSessionShortList(),
		0 ignored issues – show Documentation introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report `$shortlist = $this->getSessionShortList()` is of type `object<DataObject>`, but the function expects a `object<ShortList>\|null`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
129		$ID = $request->getVar('id'),
130		$type = $request->getVar('type'),
131		$session = $request->getVar('s')
132		);
133		break;
134		}
135
136		if ($request->isAjax()) {
137		$shortlist = $this->getSessionShortList();
138		$url = false;
139
140		if ($shortlist && $shortlist->exists()) {
141		$url = $shortlist->Link();
142		}
143
144		return json_encode(array(
145		'count' => $this->shortListCount($session),
		0 ignored issues – show Bug introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report The variable `$session` does not seem to be defined for all execution paths leading up to this point. If you define a variable conditionally, it can happen that it is not defined for all execution paths. Let’s take a look at an example: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } // $x is potentially undefined here. echo $x; } In the above example, the variable `$x` is defined if you pass “foo” or “bar” as argument for `$a`. However, since the `switch` statement has no default case statement, if you pass any other value, the variable `$x` would be undefined. Available Fixes Check for existence of the variable explicitly: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } if (isset($x)) { // Make sure it's always set. echo $x; } } Define a default value for the variable: function myFunction($a) { $x = ''; // Set a default which gets overridden for certain paths. switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } echo $x; } Add a value for the missing path: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; // We add support for the missing case. default: $x = ''; break; } echo $x; } Loading history...
146		'url' => $url
147		));
148		}
149
150		if (array_key_exists('output', $request->getVars())) {
151		return $status;
		0 ignored issues – show Bug introduced 2016-04-18 07:16 UTC by Report Bug Copy Issue Report The variable `$status` does not seem to be defined for all execution paths leading up to this point. If you define a variable conditionally, it can happen that it is not defined for all execution paths. Let’s take a look at an example: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } // $x is potentially undefined here. echo $x; } In the above example, the variable `$x` is defined if you pass “foo” or “bar” as argument for `$a`. However, since the `switch` statement has no default case statement, if you pass any other value, the variable `$x` would be undefined. Available Fixes Check for existence of the variable explicitly: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } if (isset($x)) { // Make sure it's always set. echo $x; } } Define a default value for the variable: function myFunction($a) { $x = ''; // Set a default which gets overridden for certain paths. switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } echo $x; } Add a value for the missing path: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; // We add support for the missing case. default: $x = ''; break; } echo $x; } Loading history...
152		}
153
154		return $this->redirectBack();
155		}
156
157
158		/**
159		* Get the number of items in the current short list.
160		*
161		* @param session The session to check & find a shortlist for.
162		* @return mixed false if no session exists - else the number of items in the shortlist.
163		* */
164		public function shortListCount($session = false)
165		{
166		if (is_null(session_id()) \|\| !$session \|\| $session != session_id()) {
167		return false;
168		}
169
170		$shortlist = $this->getSessionShortList();
171
172		if (!$shortlist \|\| !$shortlist->exists()) {
173		return 0;
174		}
175
176		return $shortlist->Items()->count();
177		}
178
179		private function getSessionShortList()
180		{
181		return DataObject::get_one('ShortList', $filter = array('SessionID' => session_id()), $cache = false);
182		}
183
184		public static function getShortListSession()
185		{
186		return DataObject::get_one('ShortList', $filter = array('SessionID' => session_id()));
187		}
188		}
189

salted-herring / silverstripe-shortlist

Branch — master (a3e8b1)

ShortListController::performAction() C

Complexity

Size

Duplication

Importance

How to fix Long Method Complexity

Long Method

Available Fixes

Available Fixes

Duplication Side-by-Side

Filter issues like