BlockManager - Code Metrics - Inspection of "bugfix: Missing add button. Fixes #67" - sheadawson/silverstripe-blocks - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — master ( e65159...40617e )

by Shea

created 2016-11-08 22:48 UTC

BlockManager B

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	229
Duplicated Lines	8.73 %

Coupling/Cohesion

Components	1
Dependencies	6

Importance

Changes

Metric	Value
wmc	50
lcom	1
cbo	6
dl	20
loc	229
rs	8.6206
c	0
b	0
f	0

12 Methods

Rating	Name	Duplication	Size	Complexity
A	__construct()	0	4	1
B	getAreasForTheme()	0	20	8
C	getAreasForPageType()	20	46	12
D	getBlockClasses()	0	28	9
A	getTheme()	0	12	4
A	getThemeConfig()	0	7	3
A	getUseBlockSets()	0	6	2
A	getExcludeFromPageTypes()	0	6	2
A	getWhiteListedPageTypes()	0	5	2
A	getBlackListedPageTypes()	0	7	3
A	getUseExtraCSSClasses()	0	6	2
A	getPrefixDefaultCSSClasses()	0	6	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 BlockManager 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 BlockManager, and based on these observations, apply Extract Interface, too.

<?php
/**
 * BlockManager.
 *
 * @author Shea Dawson <[email protected]>
 */
class BlockManager extends Object
namespace YourVendor;

class YourClass { }
{
	/**
	 * Define areas and config on a per theme basis.
	 *
	 * @var array
	 **/
	private static $themes = array();


	/**
	 * Use default ContentBlock class.
	 *
	 * @var bool
	 **/
	private static $use_default_blocks = true;


	/**
	 * Show a block area preview button in CMS
	 *
	 * @var bool
	 **/
	private static $block_area_preview = true;


	public function __construct()
	{
		parent::__construct();
	}

	/**
	 * Gets an array of all areas defined for the current theme.
	 *
	 * @param string $theme
	 * @param bool   $keyAsValue
	 *
	 * @return array $areas
	 **/
	public function getAreasForTheme($theme = null, $keyAsValue = true)
	{
		$theme = $theme ? $theme : $this->getTheme();
		if (!$theme) {
			return false;
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}
		}
		$config = $this->config()->get('themes');
		if (!isset($config[$theme]['areas'])) {
			return false;
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}
		}
		$areas = $config[$theme]['areas'];
		$areas = $keyAsValue ? ArrayLib::valuekey(array_keys($areas)) : $areas;
		if (count($areas)) {
			foreach ($areas as $k => $v) {
				$areas[$k] = $keyAsValue ? FormField::name_to_label($k) : $v;
			}
		}

		return $areas;
	}

	/**
	 * Gets an array of all areas defined for the current theme that are compatible
	 * with pages of type $class.
	 *
	 * @param string $class
	 *
	 * @return array $areas
	 **/
	public function getAreasForPageType($class)
	{
		$areas = $this->getAreasForTheme(null, false);

		if (!$areas) {

			return false;
		}

		foreach ($areas as $area => $config) {
			if (!is_array($config)) {
				continue;
			}

			if (isset($config['except'])) {

				$except = $config['except'];
				if (is_array($except)
					? in_array($class, $except)
					: $except == $class
				) {
					unset($areas[$area]);
					continue;
				}
			}

			if (isset($config['only'])) {

				$only = $config['only'];
				if (is_array($only)
					? !in_array($class, $only)
					: $only != $class
				) {
					unset($areas[$area]);
					continue;
				}
			}
		}

		if (count($areas)) {
			foreach ($areas as $k => $v) {
				$areas[$k] = _t('Block.BlockAreaName.'.$k, FormField::name_to_label($k));
			}

			return $areas;
		} else {
			return $areas;
		}
	}

	public function getBlockClasses()
	{
		$classes = ArrayLib::valuekey(ClassInfo::subclassesFor('Block'));
		array_shift($classes);
		foreach ($classes as $k => $v) {
			$classes[$k] = singleton($k)->singular_name();
		}

		$themeConfig = $this->getThemeConfig();

		if (isset($themeConfig['use_default_blocks']) && !$themeConfig['use_default_blocks']) {
	        unset($classes['ContentBlock']);
	    } else if (!$this->config()->use_default_blocks) {
	        unset($classes['ContentBlock']);
	    }

		$disabledArr = Config::inst()->get('BlockManager', 'disabled_blocks') ? Config::inst()->get('BlockManager', 'disabled_blocks') : array();
		if (isset($themeConfig['disabled_blocks'])) {
		    $disabledArr = array_merge($disabledArr, $themeConfig['disabled_blocks']);
		}
		if (count($disabledArr)) {
			foreach ($disabledArr as $k => $v) {
$collection = json_decode($data, true);
if ( ! is_array($collection)) {
    throw new \RuntimeException('$collection must be an array.');
}

foreach ($collection as $item) { /** ... */ }
				unset($classes[$v]);
			}
		}

		return $classes;
	}

	/*
	 * Get the current/active theme or 'default' to support theme-less sites
	 */
	private function getTheme()
	{
		$currentTheme = Config::inst()->get('SSViewer', 'theme');

		// check directly on SiteConfig incase ContentController hasn't set
		// the theme yet in ContentController->init()
		if (!$currentTheme && class_exists('SiteConfig')) {
			$currentTheme = SiteConfig::current_site_config()->Theme;
		}

		return $currentTheme ? $currentTheme : 'default';
	}

	/*
	 * Get the block config for the current theme
	 */
	private function getThemeConfig()
	{
		$theme = $this->getTheme();
		$config = $this->config()->get('themes');

		return $theme && isset($config[$theme]) ? $config[$theme] : null;
	}

	/*
	 * Usage of BlockSets configurable from yaml
	 */
	public function getUseBlockSets()
	{
		$config = $this->getThemeConfig();

		return isset($config['use_blocksets']) ? $config['use_blocksets'] : true;
	}

	/*
	 * Exclusion of blocks from page types defined in yaml
	 */
	public function getExcludeFromPageTypes()
	{
		$config = $this->getThemeConfig();

		return isset($config['exclude_from_page_types']) ? $config['exclude_from_page_types'] : array();
	}

	/*
	 * getWhiteListedPageTypes optionally configured by the developer
	 */
	public function getWhiteListedPageTypes()
	{
		$config = $this->getThemeConfig();
		return isset($config['pagetype_whitelist']) ? $config['pagetype_whitelist'] : array();
	}

	/*
	 * getBlackListedPageTypes optionally configured by the developer
	 * Includes blacklisted page types defined in the old exclude_from_page_types array
	 */
	public function getBlackListedPageTypes()
	{
		$config = $this->getThemeConfig();
		$legacy = isset($config['exclude_from_page_types']) ? $config['exclude_from_page_types'] : array();
		$current = isset($config['pagetype_blacklist']) ? $config['pagetype_blacklist'] : array();
		return array_merge($legacy, $current);
	}

	/*
	 * Usage of extra css classes configurable from yaml
	 */
	public function getUseExtraCSSClasses()
	{
		$config = $this->getThemeConfig();

		return isset($config['use_extra_css_classes']) ? $config['use_extra_css_classes'] : false;
	}

	/*
	 * Prefix for the default CSSClasses
	 */
	public function getPrefixDefaultCSSClasses()
	{
		$config = $this->getThemeConfig();

		return isset($config['prefix_default_css_classes']) ? $config['prefix_default_css_classes'] : false;
	}
}


1		<?php
2		/**
3		* BlockManager.
4		*
5		* @author Shea Dawson <[email protected]>
6		*/
7		class BlockManager extends Object
		0 ignored issues – show Coding Style Compatibility introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report PSR1 recommends that each class must be in a namespace of at least one level to avoid collisions. You can fix this by adding a namespace to your class: namespace YourVendor; class YourClass { } When choosing a vendor namespace, try to pick something that is not too generic to avoid conflicts with other libraries. Loading history...
8		{
9		/**
10		* Define areas and config on a per theme basis.
11		*
12		* @var array
13		**/
14		private static $themes = array();
		0 ignored issues – show Unused Code introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The property `$themes` 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...
15
16		/**
17		* Use default ContentBlock class.
18		*
19		* @var bool
20		**/
21		private static $use_default_blocks = true;
		0 ignored issues – show Unused Code introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The property `$use_default_blocks` 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...
22
23		/**
24		* Show a block area preview button in CMS
25		*
26		* @var bool
27		**/
28		private static $block_area_preview = true;
		0 ignored issues – show Unused Code introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The property `$block_area_preview` 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...
29
30		public function __construct()
31		{
32		parent::__construct();
33		}
34
35		/**
36		* Gets an array of all areas defined for the current theme.
37		*
38		* @param string $theme
39		* @param bool $keyAsValue
40		*
41		* @return array $areas
42		**/
43		public function getAreasForTheme($theme = null, $keyAsValue = true)
44		{
45		$theme = $theme ? $theme : $this->getTheme();
46		if (!$theme) {
47		return false;
		0 ignored issues – show Bug Best Practice introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The return type of `return false;` (`false`) is incompatible with the return type documented by `BlockManager::getAreasForTheme` of type `array`. If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design. Let’s take a look at an example: class Author { private $name; public function __construct($name) { $this->name = $name; } public function getName() { return $this->name; } } abstract class Post { public function getAuthor() { return 'Johannes'; } } class BlogPost extends Post { public function getAuthor() { return new Author('Johannes'); } } class ForumPost extends Post { /* ... */ } function my_function(Post $post) { echo strtoupper($post->getAuthor()); } Our function `my_function` expects a `Post` object, and outputs the author of the post. The base class `Post` returns a simple string and outputting a simple string will work just fine. However, the child class `BlogPost` which is a sub-type of `Post` instead decided to return an `object`, and is therefore violating the SOLID principles. If a `BlogPost` were passed to `my_function`, PHP would not complain, but ultimately fail when executing the `strtoupper` call in its body. Loading history...
48		}
49		$config = $this->config()->get('themes');
50		if (!isset($config[$theme]['areas'])) {
51		return false;
		0 ignored issues – show Bug Best Practice introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The return type of `return false;` (`false`) is incompatible with the return type documented by `BlockManager::getAreasForTheme` of type `array`. If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design. Let’s take a look at an example: class Author { private $name; public function __construct($name) { $this->name = $name; } public function getName() { return $this->name; } } abstract class Post { public function getAuthor() { return 'Johannes'; } } class BlogPost extends Post { public function getAuthor() { return new Author('Johannes'); } } class ForumPost extends Post { /* ... */ } function my_function(Post $post) { echo strtoupper($post->getAuthor()); } Our function `my_function` expects a `Post` object, and outputs the author of the post. The base class `Post` returns a simple string and outputting a simple string will work just fine. However, the child class `BlogPost` which is a sub-type of `Post` instead decided to return an `object`, and is therefore violating the SOLID principles. If a `BlogPost` were passed to `my_function`, PHP would not complain, but ultimately fail when executing the `strtoupper` call in its body. Loading history...
52		}
53		$areas = $config[$theme]['areas'];
54		$areas = $keyAsValue ? ArrayLib::valuekey(array_keys($areas)) : $areas;
55		if (count($areas)) {
56		foreach ($areas as $k => $v) {
57		$areas[$k] = $keyAsValue ? FormField::name_to_label($k) : $v;
58		}
59		}
60
61		return $areas;
62		}
63
64		/**
65		* Gets an array of all areas defined for the current theme that are compatible
66		* with pages of type $class.
67		*
68		* @param string $class
69		*
70		* @return array $areas
71		**/
72		public function getAreasForPageType($class)
73		{
74		$areas = $this->getAreasForTheme(null, false);
75
76		if (!$areas) {
		0 ignored issues – show Bug Best Practice introduced 2016-05-23 01:41 UTC by Report Bug Copy Issue Report The expression `$areas` of type `array` is implicitly converted to a boolean; are you sure this is intended? If so, consider using `empty($expr)` instead to make it clear that you intend to check for an array without elements. This check marks implicit conversions of arrays to boolean values in a comparison. While in PHP an empty array is considered to be equal (but not identical) to false, this is not always apparent. Consider making the comparison explicit by using `empty(..)` or `! empty(...)` instead. Loading history...
77		return false;
78		}
79
80		foreach ($areas as $area => $config) {
81		if (!is_array($config)) {
82		continue;
83		}
84
85	View Code Duplication	if (isset($config['except'])) {
		0 ignored issues – show Duplication introduced 2016-05-23 01:41 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...
86		$except = $config['except'];
87		if (is_array($except)
88		? in_array($class, $except)
89		: $except == $class
90		) {
91		unset($areas[$area]);
92		continue;
93		}
94		}
95
96	View Code Duplication	if (isset($config['only'])) {
		0 ignored issues – show Duplication introduced 2016-05-23 01:41 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...
97		$only = $config['only'];
98		if (is_array($only)
99		? !in_array($class, $only)
100		: $only != $class
101		) {
102		unset($areas[$area]);
103		continue;
104		}
105		}
106		}
107
108		if (count($areas)) {
109		foreach ($areas as $k => $v) {
110		$areas[$k] = _t('Block.BlockAreaName.'.$k, FormField::name_to_label($k));
111		}
112
113		return $areas;
114		} else {
115		return $areas;
116		}
117		}
118
119		public function getBlockClasses()
120		{
121		$classes = ArrayLib::valuekey(ClassInfo::subclassesFor('Block'));
122		array_shift($classes);
123		foreach ($classes as $k => $v) {
124		$classes[$k] = singleton($k)->singular_name();
125		}
126
127		$themeConfig = $this->getThemeConfig();
128
129		if (isset($themeConfig['use_default_blocks']) && !$themeConfig['use_default_blocks']) {
130		unset($classes['ContentBlock']);
131		} else if (!$this->config()->use_default_blocks) {
132		unset($classes['ContentBlock']);
133		}
134
135		$disabledArr = Config::inst()->get('BlockManager', 'disabled_blocks') ? Config::inst()->get('BlockManager', 'disabled_blocks') : array();
136		if (isset($themeConfig['disabled_blocks'])) {
137		$disabledArr = array_merge($disabledArr, $themeConfig['disabled_blocks']);
138		}
139		if (count($disabledArr)) {
140		foreach ($disabledArr as $k => $v) {
		0 ignored issues – show Bug introduced 2016-09-18 09:53 UTC by Report Bug Copy Issue Report The expression `$disabledArr` of type `array\|integer\|double\|string\|boolean` is not guaranteed to be traversable. How about adding an additional type check? There are different options of fixing this problem. If you want to be on the safe side, you can add an additional type-check: $collection = json_decode($data, true); if ( ! is_array($collection)) { throw new \RuntimeException('$collection must be an array.'); } foreach ($collection as $item) { /** ... / } If you are sure that the expression is traversable, you might want to add a doc comment cast* to improve IDE auto-completion and static analysis: /** @var array $collection / $collection = json_decode($data, true); foreach ($collection as $item) { /* .. / } Mark the issue as a false-positive*: Just hover the remove button, in the top-right corner of this issue for more options. Loading history...
141		unset($classes[$v]);
142		}
143		}
144
145		return $classes;
146		}
147
148		/*
149		* Get the current/active theme or 'default' to support theme-less sites
150		*/
151		private function getTheme()
152		{
153		$currentTheme = Config::inst()->get('SSViewer', 'theme');
154
155		// check directly on SiteConfig incase ContentController hasn't set
156		// the theme yet in ContentController->init()
157		if (!$currentTheme && class_exists('SiteConfig')) {
158		$currentTheme = SiteConfig::current_site_config()->Theme;
159		}
160
161		return $currentTheme ? $currentTheme : 'default';
162		}
163
164		/*
165		* Get the block config for the current theme
166		*/
167		private function getThemeConfig()
168		{
169		$theme = $this->getTheme();
170		$config = $this->config()->get('themes');
171
172		return $theme && isset($config[$theme]) ? $config[$theme] : null;
173		}
174
175		/*
176		* Usage of BlockSets configurable from yaml
177		*/
178		public function getUseBlockSets()
179		{
180		$config = $this->getThemeConfig();
181
182		return isset($config['use_blocksets']) ? $config['use_blocksets'] : true;
183		}
184
185		/*
186		* Exclusion of blocks from page types defined in yaml
187		*/
188		public function getExcludeFromPageTypes()
189		{
190		$config = $this->getThemeConfig();
191
192		return isset($config['exclude_from_page_types']) ? $config['exclude_from_page_types'] : array();
193		}
194
195		/*
196		* getWhiteListedPageTypes optionally configured by the developer
197		*/
198		public function getWhiteListedPageTypes()
199		{
200		$config = $this->getThemeConfig();
201		return isset($config['pagetype_whitelist']) ? $config['pagetype_whitelist'] : array();
202		}
203
204		/*
205		* getBlackListedPageTypes optionally configured by the developer
206		* Includes blacklisted page types defined in the old exclude_from_page_types array
207		*/
208		public function getBlackListedPageTypes()
209		{
210		$config = $this->getThemeConfig();
211		$legacy = isset($config['exclude_from_page_types']) ? $config['exclude_from_page_types'] : array();
212		$current = isset($config['pagetype_blacklist']) ? $config['pagetype_blacklist'] : array();
213		return array_merge($legacy, $current);
214		}
215
216		/*
217		* Usage of extra css classes configurable from yaml
218		*/
219		public function getUseExtraCSSClasses()
220		{
221		$config = $this->getThemeConfig();
222
223		return isset($config['use_extra_css_classes']) ? $config['use_extra_css_classes'] : false;
224		}
225
226		/*
227		* Prefix for the default CSSClasses
228		*/
229		public function getPrefixDefaultCSSClasses()
230		{
231		$config = $this->getThemeConfig();
232
233		return isset($config['prefix_default_css_classes']) ? $config['prefix_default_css_classes'] : false;
234		}
235		}
236

sheadawson / silverstripe-blocks

GitHub Access Token became invalid