Complex classes like UsersController 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 UsersController, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 35 | class UsersController extends AppController |
||
| 36 | { |
||
| 37 | public $helpers = [ |
||
| 38 | 'SpectrumColorpicker.SpectrumColorpicker', |
||
| 39 | 'Posting', |
||
| 40 | 'Siezi/SimpleCaptcha.SimpleCaptcha', |
||
| 41 | 'Text' |
||
| 42 | ]; |
||
| 43 | |||
| 44 | /** |
||
| 45 | * Are moderators allowed to bloack users |
||
| 46 | * |
||
| 47 | * @var bool |
||
| 48 | */ |
||
| 49 | protected $modLocking = false; |
||
| 50 | |||
| 51 | /** |
||
| 52 | * {@inheritDoc} |
||
| 53 | */ |
||
| 54 | public function initialize() |
||
| 59 | |||
| 60 | /** |
||
| 61 | * Login user. |
||
| 62 | * |
||
| 63 | * @return void|Response |
||
| 64 | */ |
||
| 65 | public function login() |
||
| 141 | |||
| 142 | /** |
||
| 143 | * Logout user. |
||
| 144 | * |
||
| 145 | * @return void|Response |
||
| 146 | */ |
||
| 147 | public function logout() |
||
| 159 | |||
| 160 | /** |
||
| 161 | * Register new user. |
||
| 162 | * |
||
| 163 | * @return void|Response |
||
| 164 | */ |
||
| 165 | public function register() |
||
| 240 | |||
| 241 | /** |
||
| 242 | * register success (user clicked link in confirm mail) |
||
| 243 | * |
||
| 244 | * @param string $id user-ID |
||
| 245 | * @return void |
||
| 246 | * @throws BadRequestException |
||
| 247 | */ |
||
| 248 | public function rs($id = null) |
||
| 264 | |||
| 265 | /** |
||
| 266 | * Show list of all users. |
||
| 267 | * |
||
| 268 | * @return void |
||
| 269 | */ |
||
| 270 | public function index() |
||
| 271 | { |
||
| 272 | $menuItems = [ |
||
| 273 | 'username' => [__('username_marking'), []], |
||
| 274 | 'user_type' => [__('user_type'), []], |
||
| 275 | 'UserOnline.logged_in' => [ |
||
| 276 | __('userlist_online'), |
||
| 277 | ['direction' => 'desc'] |
||
| 278 | ], |
||
| 279 | 'registered' => [__('registered'), ['direction' => 'desc']] |
||
| 280 | ]; |
||
| 281 | $showBlocked = Configure::read('Saito.Settings.block_user_ui'); |
||
| 282 | if ($showBlocked) { |
||
| 283 | $menuItems['user_lock'] = [ |
||
| 284 | __('user.set.lock.t'), |
||
| 285 | ['direction' => 'desc'] |
||
| 286 | ]; |
||
| 287 | } |
||
| 288 | |||
| 289 | $this->paginate = $options = [ |
||
| 290 | 'contain' => ['UserOnline'], |
||
| 291 | 'sortWhitelist' => array_keys($menuItems), |
||
| 292 | 'finder' => 'paginated', |
||
| 293 | 'limit' => 400, |
||
| 294 | 'order' => [ |
||
| 295 | 'UserOnline.logged_in' => 'desc', |
||
| 296 | ] |
||
| 297 | ]; |
||
| 298 | $users = $this->paginate($this->Users); |
||
| 299 | |||
| 300 | $showBottomNavigation = true; |
||
| 301 | |||
| 302 | $this->set(compact('menuItems', 'showBottomNavigation', 'users')); |
||
| 303 | } |
||
| 304 | |||
| 305 | /** |
||
| 306 | * Ignore user. |
||
| 307 | * |
||
| 308 | * @return void |
||
| 309 | */ |
||
| 310 | public function ignore() |
||
| 316 | |||
| 317 | /** |
||
| 318 | * Unignore user. |
||
| 319 | * |
||
| 320 | * @return void |
||
| 321 | */ |
||
| 322 | public function unignore() |
||
| 328 | |||
| 329 | /** |
||
| 330 | * Mark user as un-/ignored |
||
| 331 | * |
||
| 332 | * @param int $blockedId user to ignore |
||
| 333 | * @param bool $set block or unblock |
||
| 334 | * @return \Cake\Network\Response |
||
| 335 | */ |
||
| 336 | protected function _ignore($blockedId, $set) |
||
| 350 | |||
| 351 | /** |
||
| 352 | * Show user with profile $name |
||
| 353 | * |
||
| 354 | * @param string $name username |
||
| 355 | * @return void |
||
| 356 | */ |
||
| 357 | public function name($name = null) |
||
| 379 | |||
| 380 | /** |
||
| 381 | * View user profile. |
||
| 382 | * |
||
| 383 | * @param null $id user-ID |
||
| 384 | * @return \Cake\Network\Response|void |
||
| 385 | */ |
||
| 386 | public function view($id = null) |
||
| 387 | { |
||
| 388 | // redirect view/<username> to name/<username> |
||
| 389 | if (!empty($id) && !is_numeric($id)) { |
||
| 390 | $this->redirect( |
||
| 391 | ['controller' => 'users', 'action' => 'name', $id] |
||
| 392 | ); |
||
| 393 | |||
| 394 | return; |
||
| 395 | } |
||
| 396 | |||
| 397 | $id = (int)$id; |
||
| 398 | |||
| 399 | /** @var User */ |
||
| 400 | $user = $this->Users->find() |
||
| 401 | ->contain( |
||
| 402 | [ |
||
| 403 | 'UserBlocks' => function ($q) { |
||
| 404 | return $q->find('assocUsers'); |
||
| 405 | }, |
||
| 406 | 'UserOnline' |
||
| 407 | ] |
||
| 408 | ) |
||
| 409 | ->where(['Users.id' => (int)$id]) |
||
| 410 | ->first(); |
||
| 411 | |||
| 412 | if (empty($user)) { |
||
| 413 | $this->Flash->set(__('Invalid user'), ['element' => 'error']); |
||
| 414 | |||
| 415 | return $this->redirect('/'); |
||
| 416 | } |
||
| 417 | |||
| 418 | $entriesShownOnPage = 20; |
||
| 419 | $this->set( |
||
| 420 | 'lastEntries', |
||
| 421 | $this->Users->Entries->getRecentEntries( |
||
| 422 | $this->CurrentUser, |
||
| 423 | ['user_id' => $id, 'limit' => $entriesShownOnPage] |
||
| 424 | ) |
||
| 425 | ); |
||
| 426 | |||
| 427 | $this->set( |
||
| 428 | 'hasMoreEntriesThanShownOnPage', |
||
| 429 | ($user->numberOfPostings() - $entriesShownOnPage) > 0 |
||
| 430 | ); |
||
| 431 | |||
| 432 | if ($this->CurrentUser->getId() === $id) { |
||
| 433 | $ignores = $this->Users->UserIgnores->getAllIgnoredBy($id); |
||
| 434 | $user->set('ignores', $ignores); |
||
| 435 | } |
||
| 436 | |||
| 437 | $isEditingAllowed = $this->_isEditingAllowed($this->CurrentUser, $id); |
||
| 438 | |||
| 439 | $blockForm = new BlockForm(); |
||
| 440 | $solved = $this->Users->countSolved($id); |
||
| 441 | $this->set(compact('blockForm', 'isEditingAllowed', 'solved', 'user')); |
||
| 442 | $this->set('titleForLayout', $user->get('username')); |
||
| 443 | } |
||
| 444 | |||
| 445 | /** |
||
| 446 | * Set user avatar. |
||
| 447 | * |
||
| 448 | * @param string $userId user-ID |
||
| 449 | * @return void|\Cake\Network\Response |
||
| 450 | */ |
||
| 451 | public function avatar($userId) |
||
| 476 | |||
| 477 | /** |
||
| 478 | * Edit user. |
||
| 479 | * |
||
| 480 | * @param null $id user-ID |
||
| 481 | * |
||
| 482 | * @return \Cake\Network\Response|void |
||
| 483 | */ |
||
| 484 | public function edit($id = null) |
||
| 485 | { |
||
| 486 | $data = []; |
||
| 487 | if ($this->request->is('post') || $this->request->is('put')) { |
||
| 488 | $data = $this->request->getData(); |
||
| 489 | unset($data['id']); |
||
| 490 | //= make sure only admin can edit these fields |
||
| 491 | if (!$this->CurrentUser->permission('saito.core.user.edit')) { |
||
| 492 | // @td DRY: refactor this admin fields together with view |
||
| 493 | unset($data['username'], $data['user_email'], $data['user_type']); |
||
| 494 | } |
||
| 495 | } |
||
| 496 | $user = $this->_edit($id, $data); |
||
| 497 | if ($user === true) { |
||
| 498 | return $this->redirect(['action' => 'view', $id]); |
||
| 499 | } |
||
| 500 | |||
| 501 | $this->set('user', $user); |
||
| 502 | $this->set( |
||
| 503 | 'titleForPage', |
||
| 504 | __('user.edit.t', [$user->get('username')]) |
||
| 505 | ); |
||
| 506 | |||
| 507 | $availableThemes = $this->Themes->getAvailable($this->CurrentUser); |
||
| 508 | $availableThemes = array_combine($availableThemes, $availableThemes); |
||
| 509 | $currentTheme = $this->Themes->getThemeForUser($this->CurrentUser); |
||
| 510 | $this->set(compact('availableThemes', 'currentTheme')); |
||
| 511 | } |
||
| 512 | |||
| 513 | /** |
||
| 514 | * Handle user edit core. Retrieve user or patch if data is passed. |
||
| 515 | * |
||
| 516 | * @param string $userId user-ID |
||
| 517 | * @param array|null $data datat to update the user |
||
| 518 | * |
||
| 519 | * @return true|User true on successful save, patched user otherwise |
||
| 520 | */ |
||
| 521 | protected function _edit($userId, array $data = null) |
||
| 552 | |||
| 553 | /** |
||
| 554 | * Lock user. |
||
| 555 | * |
||
| 556 | * @return \Cake\Network\Response|void |
||
| 557 | * @throws BadRequestException |
||
| 558 | */ |
||
| 559 | public function lock() |
||
| 596 | |||
| 597 | /** |
||
| 598 | * Unblock user. |
||
| 599 | * |
||
| 600 | * @param string $id user-ID |
||
| 601 | * @return void |
||
| 602 | */ |
||
| 603 | public function unlock($id) |
||
| 621 | |||
| 622 | /** |
||
| 623 | * changes user password |
||
| 624 | * |
||
| 625 | * @param null $id user-ID |
||
| 626 | * @return void |
||
| 627 | * @throws \Saito\Exception\SaitoForbiddenException |
||
| 628 | * @throws BadRequestException |
||
| 629 | */ |
||
| 630 | public function changepassword($id = null) |
||
| 685 | |||
| 686 | /** |
||
| 687 | * Directly set password for user |
||
| 688 | * |
||
| 689 | * @param string $id user-ID |
||
| 690 | * @return Response|null |
||
| 691 | */ |
||
| 692 | public function setpassword($id) |
||
| 725 | |||
| 726 | /** |
||
| 727 | * Set slidetab-order. |
||
| 728 | * |
||
| 729 | * @return \Cake\Network\Response |
||
| 730 | * @throws BadRequestException |
||
| 731 | */ |
||
| 732 | public function slidetabOrder() |
||
| 763 | |||
| 764 | /** |
||
| 765 | * Shows user's uploads |
||
| 766 | * |
||
| 767 | * @return void |
||
| 768 | */ |
||
| 769 | public function uploads() |
||
| 772 | |||
| 773 | /** |
||
| 774 | * Set category for user. |
||
| 775 | * |
||
| 776 | * @param string|null $id category-ID |
||
| 777 | * @return \Cake\Network\Response |
||
| 778 | */ |
||
| 779 | public function setcategory(?string $id = null) |
||
| 793 | |||
| 794 | /** |
||
| 795 | * {@inheritdoc} |
||
| 796 | */ |
||
| 797 | public function beforeFilter(Event $event) |
||
| 819 | |||
| 820 | /** |
||
| 821 | * Checks if the current user is allowed to edit user $userId |
||
| 822 | * |
||
| 823 | * @param CurrentUserInterface $CurrentUser user |
||
| 824 | * @param int $userId user-ID |
||
| 825 | * @return bool |
||
| 826 | */ |
||
| 827 | protected function _isEditingAllowed(CurrentUserInterface $CurrentUser, $userId) |
||
| 835 | |||
| 836 | /** |
||
| 837 | * Logout user if logged in and create response to revisit logged out |
||
| 838 | * |
||
| 839 | * @return Response|null |
||
| 840 | */ |
||
| 841 | protected function _logoutAndComeHereAgain(): ?Response |
||
| 850 | } |
||
| 851 |
This check looks at variables that are passed out again to other methods.
If the outgoing method call has stricter type requirements than the method itself, an issue is raised.
An additional type check may prevent trouble.