ShortListController - Code Metrics - Inspection of "Namespace error" - salted-herring/silverstripe-shortlist - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — master ( 5b5552...1a2f79 )

by Simon

created 2016-04-05 00:56 UTC

ShortListController C

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	291
Duplicated Lines	23.71 %

Coupling/Cohesion

Components	1
Dependencies	0

Metric	Value
wmc	71
lcom	1
cbo	0
dl	69
loc	291
rs	5.5904

14 Methods

Rating	Name	Duplication	Size	Complexity
A	init()	0	8	2
A	index()	0	11	2
B	initList()	0	16	5
B	renderList()	0	20	10
D	add()	34	34	10
C	addToShortList()	0	27	8
D	remove()	35	35	10
B	removeFromShortList()	0	16	5
B	shortListCount()	0	14	7
B	paginatedItems()	0	23	5
A	nextPage()	0	8	2
A	prevPage()	0	8	2
A	getSessionShortList()	0	4	1
A	Link()	0	11	2

How to fix Duplicated Code Complexity

Duplicated Code

Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.

Common duplication problems, and corresponding solutions are:

If you have the same expression in different places: Extract expression to a method
If you have the same method in different sub-classes: Extract method, and pull up field to the parent class
If you have the same code in unrelated classes: Consider extracting the code to a new class, and injecting that class

Complex Class

Tip: Before tackling complexity, make sure that you eliminate any duplication first. This often can reduce the size of classes significantly.

Complex classes like ShortListController often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.

Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.

While breaking up the class, it is a good idea to analyze how other classes use ShortListController, and based on these observations, apply Extract Interface, too.

<?php
namespace SaltedHerring\ShortList;

class ShortListController extends Page_Controller
{
    private static $allowed_actions = array(

        'add',
        'remove',
        'renderList'
    );

    private static $url_handlers = array(

        'add'       => 'add',
        'remove'    => 'remove',
        '$URL!'     => 'renderList',
    );

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

        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 ($this->getSessionShortList()) {
            return $this->renderWith(
                array('Page', 'ShortList')
            );
        } else {
            // render with not found template.
            return $this->renderWith(array('Page', 'ShortList_empty'));
        }
    }

    /**
     * Create a new list if necessary. If a bot, do nothing!
     * */
    public function initList()
    {
        if (!ShortList::isBrowser()) {
            $this->httpError(404);
        }
        if ($this->request->getVar('page')) {
            $this->currentPage = $this->request->getVar('page');
        }

        $shortlist = $this->getSessionShortList();

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

    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() ||
            !ShortList::isBrowser()
        ) {
            $this->httpError(404);
        }

        return $this->customise(array(
            'ShortlistURL' => $shortlist && $shortlist->exists() ? $shortlist->Link() : false,
            'ShortlistCount' => $shortlist && $shortlist->exists() ? $shortlist->ShortListItems()->Count() : 0
        ))->renderWith(
            array('ShortList', 'Page')
        );
    }

    /**
     * Add an item to the shortlist.
     *
     * $request params:
     *
     * * type   - classname of the
     * * id     - id of the object to add.
     * * s      - session id.
     *
     * */
    public function add($request)

    {
        if (is_null(session_id()) ||
            !$request->getVar('id') ||
            !$request->getVar('type') ||
            !$request->getVar('s') ||
            $request->getVar('s') != session_id() ||
            !ShortList::isBrowser()) {
            $this->httpError(404);
        }

        $add = $this->addToShortList(
            $ID = $request->getVar('id'),
            $type = $request->getVar('type'),
            $session = $request->getVar('s')
        );

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

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

            return json_encode(array(
                'status' => $add,
                'count' => $this->ShortListCount($session),
                'url' => $url
            ));
        }

        return $add;
    }

    public function addToShortList($ID = false, $type = null, $session = false)
    {
        if (!$ID || is_null($type) || !$session) {
            return false;
        }

        $shortlist = $this->getSessionShortList();

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

        $item = DataObject::get_by_id($type, $ID);

        if ($item && $item->exists()) {
            $shortlistItem = new ShortListItem();
            $shortlistItem->ShortListID = $shortlist->ID;
            $shortlistItem->ItemID = $item->ID;
            $shortlistItem->ItemType = $type;

            $shortlist->ShortListItems()->add($shortlistItem);
            $shortlist->write();
        }

        return true;
    }

    public function remove($request)

    {
        if (is_null(session_id()) ||
            !$request->getVar('id') ||
            !$request->getVar('type') ||
            !$request->getVar('s') ||
            $request->getVar('s') != session_id() ||
            !ShortList::isBrowser()
        ) {
            $this->httpError(404);
        }

        $remove = $this->removeFromShortList(
            $ID = $request->getVar('id'),
            $type = $request->getVar('type'),
            $session = $request->getVar('s')
        );

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

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

            return json_encode(array(
                'status' => $remove,
                'count' => $this->shortListCount($session),
                'url' => $url
            ));
        }

        return $remove;
    }

    private function removeFromShortList($ID = false, $type = null, $session = false)
    {
        $shortlist = $this->getSessionShortList();

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

        $item = DataObject::get_one('ShortListItem', $filter = "ItemType = '" . $type . "' AND ItemID = " . $ID);

        if ($item && $item->exists()) {
            $item->delete();
        }

        return true;
    }

    /**
     * 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() || !ShortList::isBrowser()) {
            return false;
        }

        $shortlist = $this->getSessionShortList();

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

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


    public function paginatedItems()
    {
        if (!$this->getRequest()->param('URL') || !ShortList::isBrowser()) {
            return false;
        }

        $items = false;
        $list = DataObject::get_one('ShortList', $filter = array('URL' => $this->getRequest()->param('URL')));

        if ($list) {
            $items = $list->ShortListItems();
        }

        $this->list = new PaginatedList($items, $this->getRequest());
        $this->list->setPageLength(Config::inst()->get('ShortList', 'PaginationCount'));
        $this->list->setPaginationGetVar('page');

        if ($this->currentPage) {
            $this->list->setCurrentPage($this->currentPage);
        }

        return $this->list;
    }

    public function nextPage()
    {
        if ($this->list->CurrentPage() < $this->list->TotalPages()) {
            return '?page=' . ($this->list->CurrentPage() + 1);
        }

        return false;
    }

    public function prevPage()
    {
        if ($this->list->CurrentPage() > 1) {
            return '?page=' . ($this->list->CurrentPage() - 1);
        }

        return false;
    }


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

    /**
     * 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;
    }
}


1		<?php
2		namespace SaltedHerring\ShortList;
3
4		class ShortListController extends Page_Controller
5		{
6		private static $allowed_actions = array(
		1 ignored issue – show Unused Code introduced 2016-04-05 00:45 UTC by Report Bug Copy Issue Report The property `$allowed_actions` is not used and could be removed. This check marks private properties in classes that are never used. Those properties can be removed. Loading history...
7		'add',
8		'remove',
9		'renderList'
10		);
11
12		private static $url_handlers = array(
		1 ignored issue – show Unused Code introduced 2016-04-05 00:45 UTC by Report Bug Copy Issue Report The property `$url_handlers` is not used and could be removed. This check marks private properties in classes that are never used. Those properties can be removed. Loading history...
13		'add' => 'add',
14		'remove' => 'remove',
15		'$URL!' => 'renderList',
16		);
17
18		public function init()
19		{
20		parent::init();
21
22		if ($this->request->getVar('page')) {
23		$this->currentPage = $this->request->getVar('page');
24		}
25		}
26
27		/**
28		* When landing on the homepage, if there is a shortlist for the current
29		* user, redirect to the correct URL. Otherwise, 404.
30		* */
31		public function index($request)
32		{
33		if ($this->getSessionShortList()) {
34		return $this->renderWith(
35		array('Page', 'ShortList')
36		);
37		} else {
38		// render with not found template.
39		return $this->renderWith(array('Page', 'ShortList_empty'));
40		}
41		}
42
43		/**
44		* Create a new list if necessary. If a bot, do nothing!
45		* */
46		public function initList()
47		{
48		if (!ShortList::isBrowser()) {
49		$this->httpError(404);
50		}
51		if ($this->request->getVar('page')) {
52		$this->currentPage = $this->request->getVar('page');
53		}
54
55		$shortlist = $this->getSessionShortList();
56
57		if (!$shortlist \|\| !$shortlist->exists()) {
58		$shortlist = new ShortList();
59		$shortlist->write();
60		}
61		}
62
63		public function renderList($request)
64		{
65		$shortlist = DataObject::get_one('ShortList', $filter = array('URL' => $request->param('URL')));
66
67		if (is_null(session_id()) \|\|
68		!$request->param('URL') \|\|
69		!$shortlist \|\|
70		!$shortlist->exists() \|\|
71		!ShortList::isBrowser()
72		) {
73		$this->httpError(404);
74		}
75
76		return $this->customise(array(
77		'ShortlistURL' => $shortlist && $shortlist->exists() ? $shortlist->Link() : false,
78		'ShortlistCount' => $shortlist && $shortlist->exists() ? $shortlist->ShortListItems()->Count() : 0
79		))->renderWith(
80		array('ShortList', 'Page')
81		);
82		}
83
84		/**
85		* Add an item to the shortlist.
86		*
87		* $request params:
88		*
89		* * type - classname of the
90		* * id - id of the object to add.
91		* * s - session id.
92		*
93		* */
94	View Code Duplication	public function add($request)
		0 ignored issues – show Duplication introduced 2016-04-05 00:45 UTC by Report Bug Copy Issue Report This method seems to be duplicated in 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...
95		{
96		if (is_null(session_id()) \|\|
97		!$request->getVar('id') \|\|
98		!$request->getVar('type') \|\|
99		!$request->getVar('s') \|\|
100		$request->getVar('s') != session_id() \|\|
101		!ShortList::isBrowser()) {
102		$this->httpError(404);
103		}
104
105		$add = $this->addToShortList(
106		$ID = $request->getVar('id'),
107		$type = $request->getVar('type'),
108		$session = $request->getVar('s')
109		);
110
111		if ($request->isAjax()) {
112		$shortlist = $this->getSessionShortList();
113		$url = false;
114
115		if ($shortlist && $shortlist->exists()) {
116		$url = $shortlist->Link();
117		}
118
119		return json_encode(array(
120		'status' => $add,
121		'count' => $this->ShortListCount($session),
122		'url' => $url
123		));
124		}
125
126		return $add;
127		}
128
129		public function addToShortList($ID = false, $type = null, $session = false)
130		{
131		if (!$ID \|\| is_null($type) \|\| !$session) {
132		return false;
133		}
134
135		$shortlist = $this->getSessionShortList();
136
137		if (!$shortlist \|\| !$shortlist->exists()) {
138		$shortlist = new ShortList();
139		$shortlist->write();
140		}
141
142		$item = DataObject::get_by_id($type, $ID);
143
144		if ($item && $item->exists()) {
145		$shortlistItem = new ShortListItem();
146		$shortlistItem->ShortListID = $shortlist->ID;
147		$shortlistItem->ItemID = $item->ID;
148		$shortlistItem->ItemType = $type;
149
150		$shortlist->ShortListItems()->add($shortlistItem);
151		$shortlist->write();
152		}
153
154		return true;
155		}
156
157	View Code Duplication	public function remove($request)
		0 ignored issues – show Duplication introduced 2016-04-05 00:45 UTC by Report Bug Copy Issue Report This method seems to be duplicated in 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...
158		{
159		if (is_null(session_id()) \|\|
160		!$request->getVar('id') \|\|
161		!$request->getVar('type') \|\|
162		!$request->getVar('s') \|\|
163		$request->getVar('s') != session_id() \|\|
164		!ShortList::isBrowser()
165		) {
166		$this->httpError(404);
167		}
168
169		$remove = $this->removeFromShortList(
170		$ID = $request->getVar('id'),
171		$type = $request->getVar('type'),
172		$session = $request->getVar('s')
173		);
174
175		if ($request->isAjax()) {
176		$shortlist = $this->getSessionShortList();
177		$url = '#';
178
179		if ($shortlist && $shortlist->exists()) {
180		$url = $shortlist->Link();
181		}
182
183		return json_encode(array(
184		'status' => $remove,
185		'count' => $this->shortListCount($session),
186		'url' => $url
187		));
188		}
189
190		return $remove;
191		}
192
193		private function removeFromShortList($ID = false, $type = null, $session = false)
194		{
195		$shortlist = $this->getSessionShortList();
196
197		if (!$shortlist \|\| !$shortlist->exists()) {
198		return true;
199		}
200
201		$item = DataObject::get_one('ShortListItem', $filter = "ItemType = '" . $type . "' AND ItemID = " . $ID);
202
203		if ($item && $item->exists()) {
204		$item->delete();
205		}
206
207		return true;
208		}
209
210		/**
211		* Get the number of items in the current short list.
212		*
213		* @param session The session to check & find a shortlist for.
214		* @return mixed false if no session exists - else the number of items in the shortlist.
215		* */
216		public function shortListCount($session = false)
217		{
218		if (is_null(session_id()) \|\| !$session \|\| $session != session_id() \|\| !ShortList::isBrowser()) {
219		return false;
220		}
221
222		$shortlist = $this->getSessionShortList();
223
224		if (!$shortlist \|\| !$shortlist->exists()) {
225		return 0;
226		}
227
228		return $shortlist->Items()->count();
229		}
230
231
232		public function paginatedItems()
233		{
234		if (!$this->getRequest()->param('URL') \|\| !ShortList::isBrowser()) {
235		return false;
236		}
237
238		$items = false;
239		$list = DataObject::get_one('ShortList', $filter = array('URL' => $this->getRequest()->param('URL')));
240
241		if ($list) {
242		$items = $list->ShortListItems();
243		}
244
245		$this->list = new PaginatedList($items, $this->getRequest());
246		$this->list->setPageLength(Config::inst()->get('ShortList', 'PaginationCount'));
247		$this->list->setPaginationGetVar('page');
248
249		if ($this->currentPage) {
250		$this->list->setCurrentPage($this->currentPage);
251		}
252
253		return $this->list;
254		}
255
256		public function nextPage()
257		{
258		if ($this->list->CurrentPage() < $this->list->TotalPages()) {
259		return '?page=' . ($this->list->CurrentPage() + 1);
260		}
261
262		return false;
263		}
264
265		public function prevPage()
266		{
267		if ($this->list->CurrentPage() > 1) {
268		return '?page=' . ($this->list->CurrentPage() - 1);
269		}
270
271		return false;
272		}
273
274
275		private function getSessionShortList()
276		{
277		return DataObject::get_one('ShortList', $filter = array('SessionID' => session_id()));
278		}
279
280		/**
281		* Get the absolute URL of this controller.
282		* */
283		public function Link($action = null)
284		{
285		$shortlist = $this->getSessionShortList();
286		$url = Config::inst()->get('ShortList', 'URLSegment');
287
288		if ($shortlist) {
289		$url .= $shortlist->URL;
290		}
291
292		return $url;
293		}
294		}
295

salted-herring / silverstripe-shortlist