sminnee /
silverstripe-framework
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 RequestHandler 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 RequestHandler, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 48 | class RequestHandler extends ViewableData |
||
| 49 | { |
||
| 50 | /** |
||
| 51 | * Optional url_segment for this request handler |
||
| 52 | * |
||
| 53 | * @config |
||
| 54 | * @var string|null |
||
| 55 | */ |
||
| 56 | private static $url_segment = null; |
||
| 57 | |||
| 58 | /** |
||
| 59 | * @var HTTPRequest $request The request object that the controller was called with. |
||
| 60 | * Set in {@link handleRequest()}. Useful to generate the {} |
||
| 61 | */ |
||
| 62 | protected $request = null; |
||
| 63 | |||
| 64 | /** |
||
| 65 | * The DataModel for this request |
||
| 66 | */ |
||
| 67 | protected $model = null; |
||
| 68 | |||
| 69 | /** |
||
| 70 | * This variable records whether RequestHandler::__construct() |
||
| 71 | * was called or not. Useful for checking if subclasses have |
||
| 72 | * called parent::__construct() |
||
| 73 | * |
||
| 74 | * @var boolean |
||
| 75 | */ |
||
| 76 | protected $brokenOnConstruct = true; |
||
| 77 | |||
| 78 | /** |
||
| 79 | * The default URL handling rules. This specifies that the next component of the URL corresponds to a method to |
||
| 80 | * be called on this RequestHandlingData object. |
||
| 81 | * |
||
| 82 | * The keys of this array are parse rules. See {@link HTTPRequest::match()} for a description of the rules |
||
| 83 | * available. |
||
| 84 | * |
||
| 85 | * The values of the array are the method to be called if the rule matches. If this value starts with a '$', then |
||
| 86 | * the named parameter of the parsed URL wil be used to determine the method name. |
||
| 87 | * @config |
||
| 88 | */ |
||
| 89 | private static $url_handlers = array( |
||
| 90 | '$Action' => '$Action', |
||
| 91 | ); |
||
| 92 | |||
| 93 | |||
| 94 | /** |
||
| 95 | * Define a list of action handling methods that are allowed to be called directly by URLs. |
||
| 96 | * The variable should be an array of action names. This sample shows the different values that it can contain: |
||
| 97 | * |
||
| 98 | * <code> |
||
| 99 | * array( |
||
| 100 | * // someaction can be accessed by anyone, any time |
||
| 101 | * 'someaction', |
||
| 102 | * // So can otheraction |
||
| 103 | * 'otheraction' => true, |
||
| 104 | * // restrictedaction can only be people with ADMIN privilege |
||
| 105 | * 'restrictedaction' => 'ADMIN', |
||
| 106 | * // complexaction can only be accessed if $this->canComplexAction() returns true |
||
| 107 | * 'complexaction' '->canComplexAction' |
||
| 108 | * ); |
||
| 109 | * </code> |
||
| 110 | * |
||
| 111 | * Form getters count as URL actions as well, and should be included in allowed_actions. |
||
| 112 | * Form actions on the other handed (first argument to {@link FormAction()} should NOT be included, |
||
| 113 | * these are handled separately through {@link Form->httpSubmission}. You can control access on form actions |
||
| 114 | * either by conditionally removing {@link FormAction} in the form construction, |
||
| 115 | * or by defining $allowed_actions in your {@link Form} class. |
||
| 116 | * @config |
||
| 117 | */ |
||
| 118 | private static $allowed_actions = null; |
||
| 119 | |||
| 120 | public function __construct() |
||
| 131 | |||
| 132 | /** |
||
| 133 | * Set the DataModel for this request. |
||
| 134 | * |
||
| 135 | * @param DataModel $model |
||
| 136 | */ |
||
| 137 | public function setDataModel($model) |
||
| 141 | |||
| 142 | /** |
||
| 143 | * Handles URL requests. |
||
| 144 | * |
||
| 145 | * - ViewableData::handleRequest() iterates through each rule in {@link self::$url_handlers}. |
||
| 146 | * - If the rule matches, the named method will be called. |
||
| 147 | * - If there is still more URL to be processed, then handleRequest() |
||
| 148 | * is called on the object that that method returns. |
||
| 149 | * |
||
| 150 | * Once all of the URL has been processed, the final result is returned. |
||
| 151 | * However, if the final result is an array, this |
||
| 152 | * array is interpreted as being additional template data to customise the |
||
| 153 | * 2nd to last result with, rather than an object |
||
| 154 | * in its own right. This is most frequently used when a Controller's |
||
| 155 | * action will return an array of data with which to |
||
| 156 | * customise the controller. |
||
| 157 | * |
||
| 158 | * @param HTTPRequest $request The object that is reponsible for distributing URL parsing |
||
| 159 | * @param DataModel $model |
||
| 160 | * @return HTTPResponse|RequestHandler|string|array |
||
| 161 | */ |
||
| 162 | public function handleRequest(HTTPRequest $request, DataModel $model) |
||
| 258 | |||
| 259 | /** |
||
| 260 | * @param HTTPRequest $request |
||
| 261 | * @return array |
||
| 262 | */ |
||
| 263 | protected function findAction($request) |
||
| 298 | |||
| 299 | /** |
||
| 300 | * @param string $link |
||
| 301 | * @return string |
||
| 302 | */ |
||
| 303 | View Code Duplication | protected function addBackURLParam($link) |
|
| 312 | |||
| 313 | /** |
||
| 314 | * Given a request, and an action name, call that action name on this RequestHandler |
||
| 315 | * |
||
| 316 | * Must not raise HTTPResponse_Exceptions - instead it should return |
||
| 317 | * |
||
| 318 | * @param $request |
||
| 319 | * @param $action |
||
| 320 | * @return HTTPResponse |
||
| 321 | */ |
||
| 322 | protected function handleAction($request, $action) |
||
| 344 | |||
| 345 | /** |
||
| 346 | * Get a array of allowed actions defined on this controller, |
||
| 347 | * any parent classes or extensions. |
||
| 348 | * |
||
| 349 | * Caution: Since 3.1, allowed_actions definitions only apply |
||
| 350 | * to methods on the controller they're defined on, |
||
| 351 | * so it is recommended to use the $class argument |
||
| 352 | * when invoking this method. |
||
| 353 | * |
||
| 354 | * @param string $limitToClass |
||
| 355 | * @return array|null |
||
| 356 | */ |
||
| 357 | public function allowedActions($limitToClass = null) |
||
| 385 | |||
| 386 | /** |
||
| 387 | * Checks if this request handler has a specific action, |
||
| 388 | * even if the current user cannot access it. |
||
| 389 | * Includes class ancestry and extensions in the checks. |
||
| 390 | * |
||
| 391 | * @param string $action |
||
| 392 | * @return bool |
||
| 393 | */ |
||
| 394 | public function hasAction($action) |
||
| 436 | |||
| 437 | /** |
||
| 438 | * Return the class that defines the given action, so that we know where to check allowed_actions. |
||
| 439 | * |
||
| 440 | * @param string $actionOrigCasing |
||
| 441 | * @return string |
||
| 442 | */ |
||
| 443 | protected function definingClassForAction($actionOrigCasing) |
||
| 459 | |||
| 460 | /** |
||
| 461 | * Check that the given action is allowed to be called from a URL. |
||
| 462 | * It will interrogate {@link self::$allowed_actions} to determine this. |
||
| 463 | * |
||
| 464 | * @param string $action |
||
| 465 | * @return bool |
||
| 466 | * @throws Exception |
||
| 467 | */ |
||
| 468 | public function checkAccessAction($action) |
||
| 518 | |||
| 519 | /** |
||
| 520 | * Throws a HTTP error response encased in a {@link HTTPResponse_Exception}, which is later caught in |
||
| 521 | * {@link RequestHandler::handleAction()} and returned to the user. |
||
| 522 | * |
||
| 523 | * @param int $errorCode |
||
| 524 | * @param string $errorMessage Plaintext error message |
||
| 525 | * @uses HTTPResponse_Exception |
||
| 526 | * @throws HTTPResponse_Exception |
||
| 527 | */ |
||
| 528 | public function httpError($errorCode, $errorMessage = null) |
||
| 541 | |||
| 542 | /** |
||
| 543 | * Returns the HTTPRequest object that this controller is using. |
||
| 544 | * Returns a placeholder {@link NullHTTPRequest} object unless |
||
| 545 | * {@link handleAction()} or {@link handleRequest()} have been called, |
||
| 546 | * which adds a reference to an actual {@link HTTPRequest} object. |
||
| 547 | * |
||
| 548 | * @return HTTPRequest |
||
| 549 | */ |
||
| 550 | public function getRequest() |
||
| 554 | |||
| 555 | /** |
||
| 556 | * Typically the request is set through {@link handleAction()} |
||
| 557 | * or {@link handleRequest()}, but in some based we want to set it manually. |
||
| 558 | * |
||
| 559 | * @param HTTPRequest $request |
||
| 560 | * @return $this |
||
| 561 | */ |
||
| 562 | public function setRequest($request) |
||
| 567 | |||
| 568 | /** |
||
| 569 | * Returns a link to this controller. Overload with your own Link rules if they exist. |
||
| 570 | * |
||
| 571 | * @param string $action Optional action |
||
| 572 | * @return string |
||
| 573 | */ |
||
| 574 | public function Link($action = null) |
||
| 590 | |||
| 591 | /** |
||
| 592 | * Redirect to the given URL. |
||
| 593 | * |
||
| 594 | * @param string $url |
||
| 595 | * @param int $code |
||
| 596 | * @return HTTPResponse |
||
| 597 | */ |
||
| 598 | public function redirect($url, $code = 302) |
||
| 604 | |||
| 605 | /** |
||
| 606 | * Safely get the value of the BackURL param, if provided via querystring / posted var |
||
| 607 | * |
||
| 608 | * @return string |
||
| 609 | */ |
||
| 610 | public function getBackURL() |
||
| 629 | |||
| 630 | /** |
||
| 631 | * Returns the referer, if it is safely validated as an internal URL |
||
| 632 | * and can be redirected to. |
||
| 633 | * |
||
| 634 | * @internal called from {@see Form::getValidationErrorResponse} |
||
| 635 | * @return string|null |
||
| 636 | */ |
||
| 637 | public function getReturnReferer() |
||
| 645 | |||
| 646 | /** |
||
| 647 | * Get referer |
||
| 648 | * |
||
| 649 | * @return string |
||
| 650 | */ |
||
| 651 | public function getReferer() |
||
| 659 | |||
| 660 | /** |
||
| 661 | * Redirect back. Uses either the HTTP-Referer or a manually set request-variable called "BackURL". |
||
| 662 | * This variable is needed in scenarios where HTTP-Referer is not sent (e.g when calling a page by |
||
| 663 | * location.href in IE). If none of the two variables is available, it will redirect to the base |
||
| 664 | * URL (see {@link Director::baseURL()}). |
||
| 665 | * |
||
| 666 | * @uses redirect() |
||
| 667 | * |
||
| 668 | * @return HTTPResponse |
||
| 669 | */ |
||
| 670 | public function redirectBack() |
||
| 685 | } |
||
| 686 |
Loading history...
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.