gctools-outilsgc /
gcconnex
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:
Complex classes like ElggPluginPackage 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 ElggPluginPackage, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 19 | class ElggPluginPackage { |
||
| 20 | |||
| 21 | /** |
||
| 22 | * The required files in the package |
||
| 23 | * |
||
| 24 | * @var array |
||
| 25 | */ |
||
| 26 | private $requiredFiles = array( |
||
| 27 | 'start.php', 'manifest.xml' |
||
| 28 | ); |
||
| 29 | |||
| 30 | /** |
||
| 31 | * The optional files that can be read and served through the markdown page handler |
||
| 32 | * @var array |
||
| 33 | */ |
||
| 34 | private $textFiles = array( |
||
| 35 | 'README.txt', 'CHANGES.txt', |
||
| 36 | 'INSTALL.txt', 'COPYRIGHT.txt', 'LICENSE.txt', |
||
| 37 | |||
| 38 | 'README', 'README.md', 'README.markdown' |
||
| 39 | ); |
||
| 40 | |||
| 41 | /** |
||
| 42 | * Valid types for provides. |
||
| 43 | * |
||
| 44 | * @var array |
||
| 45 | */ |
||
| 46 | private $providesSupportedTypes = array( |
||
| 47 | 'plugin', 'php_extension' |
||
| 48 | ); |
||
| 49 | |||
| 50 | /** |
||
| 51 | * The type of requires/conflicts supported |
||
| 52 | * |
||
| 53 | * @var array |
||
| 54 | */ |
||
| 55 | private $depsSupportedTypes = array( |
||
| 56 | 'elgg_version', 'elgg_release', 'php_version', 'php_extension', 'php_ini', 'plugin', 'priority', |
||
| 57 | ); |
||
| 58 | |||
| 59 | /** |
||
| 60 | * An invalid plugin error. |
||
| 61 | */ |
||
| 62 | private $errorMsg = ''; |
||
| 63 | |||
| 64 | /** |
||
| 65 | * The plugin's manifest object |
||
| 66 | * |
||
| 67 | * @var \ElggPluginManifest |
||
| 68 | */ |
||
| 69 | protected $manifest; |
||
| 70 | |||
| 71 | /** |
||
| 72 | * The plugin's full path |
||
| 73 | * |
||
| 74 | * @var string |
||
| 75 | */ |
||
| 76 | protected $path; |
||
| 77 | |||
| 78 | /** |
||
| 79 | * Is the plugin valid? |
||
| 80 | * |
||
| 81 | * @var mixed Bool after validation check, null before. |
||
| 82 | */ |
||
| 83 | protected $valid = null; |
||
| 84 | |||
| 85 | /** |
||
| 86 | * The plugin ID (dir name) |
||
| 87 | * |
||
| 88 | * @var string |
||
| 89 | */ |
||
| 90 | protected $id; |
||
| 91 | |||
| 92 | /** |
||
| 93 | * Load a plugin package from mod/$id or by full path. |
||
| 94 | * |
||
| 95 | * @param string $plugin The ID (directory name) or full path of the plugin. |
||
| 96 | * @param bool $validate Automatically run isValid()? |
||
| 97 | * |
||
| 98 | * @throws PluginException |
||
| 99 | */ |
||
| 100 | public function __construct($plugin, $validate = true) { |
||
| 138 | |||
| 139 | /******************************** |
||
| 140 | * Validation and sanity checks * |
||
| 141 | ********************************/ |
||
| 142 | |||
| 143 | /** |
||
| 144 | * Checks if this is a valid Elgg plugin. |
||
| 145 | * |
||
| 146 | * Checks for requires files as defined at the start of this |
||
| 147 | * class. Will check require manifest fields via \ElggPluginManifest |
||
| 148 | * for Elgg 1.8 plugins. |
||
| 149 | * |
||
| 150 | * @note This doesn't check dependencies or conflicts. |
||
| 151 | * Use {@link \ElggPluginPackage::canActivate()} or |
||
| 152 | * {@link \ElggPluginPackage::checkDependencies()} for that. |
||
| 153 | * |
||
| 154 | * @return bool |
||
| 155 | */ |
||
| 156 | public function isValid() { |
||
| 162 | |||
| 163 | /** |
||
| 164 | * @return bool |
||
| 165 | */ |
||
| 166 | private function validate() { |
||
| 200 | |||
| 201 | /** |
||
| 202 | * Check that the plugin is installed in the directory with name specified |
||
| 203 | * in the manifest's "id" element. |
||
| 204 | * |
||
| 205 | * @return bool |
||
| 206 | */ |
||
| 207 | private function isNamedCorrectly() { |
||
| 219 | |||
| 220 | /** |
||
| 221 | * Check the plugin doesn't require or conflict with itself |
||
| 222 | * or something provides. Also check that it only list |
||
| 223 | * valid provides. Deps are checked in checkDependencies() |
||
| 224 | * |
||
| 225 | * @note Plugins always provide themselves. |
||
| 226 | * |
||
| 227 | * @todo Don't let them require and conflict the same thing |
||
| 228 | * |
||
| 229 | * @return bool |
||
| 230 | */ |
||
| 231 | private function hasSaneDependencies() { |
||
| 278 | |||
| 279 | |||
| 280 | /************ |
||
| 281 | * Manifest * |
||
| 282 | ************/ |
||
| 283 | |||
| 284 | /** |
||
| 285 | * Returns a parsed manifest file. |
||
| 286 | * |
||
| 287 | * @return \ElggPluginManifest |
||
| 288 | */ |
||
| 289 | public function getManifest() { |
||
| 298 | |||
| 299 | /** |
||
| 300 | * Loads the manifest into this->manifest as an |
||
| 301 | * \ElggPluginManifest object. |
||
| 302 | * |
||
| 303 | * @return bool |
||
| 304 | */ |
||
| 305 | private function loadManifest() { |
||
| 322 | |||
| 323 | /**************** |
||
| 324 | * Readme Files * |
||
| 325 | ***************/ |
||
| 326 | |||
| 327 | /** |
||
| 328 | * Returns an array of present and readable text files |
||
| 329 | * |
||
| 330 | * @return array |
||
| 331 | */ |
||
| 332 | public function getTextFilenames() { |
||
| 335 | |||
| 336 | /*********************** |
||
| 337 | * Dependencies system * |
||
| 338 | ***********************/ |
||
| 339 | |||
| 340 | /** |
||
| 341 | * Returns if the Elgg system meets the plugin's dependency |
||
| 342 | * requirements. This includes both requires and conflicts. |
||
| 343 | * |
||
| 344 | * Full reports can be requested. The results are returned |
||
| 345 | * as an array of arrays in the form array( |
||
| 346 | * 'type' => requires|conflicts, |
||
| 347 | * 'dep' => array( dependency array ), |
||
| 348 | * 'status' => bool if depedency is met, |
||
| 349 | * 'comment' => optional comment to display to the user. |
||
| 350 | * ) |
||
| 351 | * |
||
| 352 | * @param bool $full_report Return a full report. |
||
| 353 | * @return bool|array |
||
| 354 | */ |
||
| 355 | public function checkDependencies($full_report = false) { |
||
| 476 | |||
| 477 | /** |
||
| 478 | * Checks if $plugins meets the requirement by $dep. |
||
| 479 | * |
||
| 480 | * @param array $dep An Elgg manifest.xml deps array |
||
| 481 | * @param array $plugins A list of plugins as returned by elgg_get_plugins(); |
||
| 482 | * @param bool $inverse Inverse the results to use as a conflicts. |
||
| 483 | * @return bool |
||
| 484 | */ |
||
| 485 | private function checkDepPlugin(array $dep, array $plugins, $inverse = false) { |
||
| 494 | |||
| 495 | /** |
||
| 496 | * Checks if $plugins meets the requirement by $dep. |
||
| 497 | * |
||
| 498 | * @param array $dep An Elgg manifest.xml deps array |
||
| 499 | * @param array $plugins A list of plugins as returned by elgg_get_plugins(); |
||
| 500 | * @param bool $inverse Inverse the results to use as a conflicts. |
||
| 501 | * @return bool |
||
| 502 | */ |
||
| 503 | private function checkDepPriority(array $dep, array $plugins, $inverse = false) { |
||
| 556 | |||
| 557 | /** |
||
| 558 | * Checks if $elgg_version meets the requirement by $dep. |
||
| 559 | * |
||
| 560 | * @param array $dep An Elgg manifest.xml deps array |
||
| 561 | * @param array $elgg_version An Elgg version (either YYYYMMDDXX or X.Y.Z) |
||
| 562 | * @param bool $inverse Inverse the result to use as a conflicts. |
||
| 563 | * @return bool |
||
| 564 | */ |
||
| 565 | View Code Duplication | private function checkDepElgg(array $dep, $elgg_version, $inverse = false) { |
|
| 577 | |||
| 578 | /** |
||
| 579 | * Checks if $php_version meets the requirement by $dep. |
||
| 580 | * |
||
| 581 | * @param array $dep An Elgg manifest.xml deps array |
||
| 582 | * @param bool $inverse Inverse the result to use as a conflicts. |
||
| 583 | * @return bool |
||
| 584 | */ |
||
| 585 | View Code Duplication | private function checkDepPhpVersion(array $dep, $inverse = false) { |
|
| 598 | |||
| 599 | /** |
||
| 600 | * Checks if the PHP extension in $dep is loaded. |
||
| 601 | * |
||
| 602 | * @todo Can this be merged with the plugin checker? |
||
| 603 | * |
||
| 604 | * @param array $dep An Elgg manifest.xml deps array |
||
| 605 | * @param bool $inverse Inverse the result to use as a conflicts. |
||
| 606 | * @return array An array in the form array( |
||
| 607 | * 'status' => bool |
||
| 608 | * 'value' => string The version provided |
||
| 609 | * ) |
||
| 610 | */ |
||
| 611 | private function checkDepPhpExtension(array $dep, $inverse = false) { |
||
| 650 | |||
| 651 | /** |
||
| 652 | * Check if the PHP ini setting satisfies $dep. |
||
| 653 | * |
||
| 654 | * @param array $dep An Elgg manifest.xml deps array |
||
| 655 | * @param bool $inverse Inverse the result to use as a conflicts. |
||
| 656 | * @return bool |
||
| 657 | */ |
||
| 658 | private function checkDepPhpIni($dep, $inverse = false) { |
||
| 683 | |||
| 684 | /** |
||
| 685 | * Returns the Plugin ID |
||
| 686 | * |
||
| 687 | * @return string |
||
| 688 | */ |
||
| 689 | public function getID() { |
||
| 692 | |||
| 693 | /** |
||
| 694 | * Returns the last error message. |
||
| 695 | * |
||
| 696 | * @return string |
||
| 697 | */ |
||
| 698 | public function getError() { |
||
| 701 | } |
||
| 702 |
Loading history...
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:
Our function
my_functionexpects aPostobject, and outputs the author of the post. The base classPostreturns a simple string and outputting a simple string will work just fine. However, the child classBlogPostwhich is a sub-type ofPostinstead decided to return anobject, and is therefore violating the SOLID principles. If aBlogPostwere passed tomy_function, PHP would not complain, but ultimately fail when executing thestrtouppercall in its body.