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 |
||
37 | class UsersController extends AppController |
||
38 | { |
||
39 | public $helpers = [ |
||
40 | 'SpectrumColorpicker.SpectrumColorpicker', |
||
41 | 'Posting', |
||
42 | 'Siezi/SimpleCaptcha.SimpleCaptcha', |
||
43 | 'Text' |
||
44 | ]; |
||
45 | |||
46 | /** |
||
47 | * {@inheritDoc} |
||
48 | */ |
||
49 | public function initialize() |
||
54 | |||
55 | /** |
||
56 | * Login user. |
||
57 | * |
||
58 | * @return void|Response |
||
59 | */ |
||
60 | public function login() |
||
134 | |||
135 | /** |
||
136 | * Logout user. |
||
137 | * |
||
138 | * @return void|Response |
||
139 | */ |
||
140 | public function logout() |
||
152 | |||
153 | /** |
||
154 | * Register new user. |
||
155 | * |
||
156 | * @return void|Response |
||
157 | */ |
||
158 | public function register() |
||
233 | |||
234 | /** |
||
235 | * register success (user clicked link in confirm mail) |
||
236 | * |
||
237 | * @param string $id user-ID |
||
238 | * @return void |
||
239 | * @throws BadRequestException |
||
240 | */ |
||
241 | public function rs($id = null) |
||
257 | |||
258 | /** |
||
259 | * Show list of all users. |
||
260 | * |
||
261 | * @return void |
||
262 | */ |
||
263 | public function index() |
||
264 | { |
||
265 | $menuItems = [ |
||
266 | 'username' => [__('username_marking'), []], |
||
267 | 'user_type' => [__('user_type'), []], |
||
268 | 'UserOnline.logged_in' => [ |
||
269 | __('userlist_online'), |
||
270 | ['direction' => 'desc'] |
||
271 | ], |
||
272 | 'registered' => [__('registered'), ['direction' => 'desc']] |
||
273 | ]; |
||
274 | $showBlocked = $this->CurrentUser->permission('saito.core.user.lock.view'); |
||
275 | if ($showBlocked) { |
||
276 | $menuItems['user_lock'] = [ |
||
277 | __('user.set.lock.t'), |
||
278 | ['direction' => 'desc'] |
||
279 | ]; |
||
280 | } |
||
281 | |||
282 | $this->paginate = $options = [ |
||
283 | 'contain' => ['UserOnline'], |
||
284 | 'sortWhitelist' => array_keys($menuItems), |
||
285 | 'finder' => 'paginated', |
||
286 | 'limit' => 400, |
||
287 | 'order' => [ |
||
288 | 'UserOnline.logged_in' => 'desc', |
||
289 | ] |
||
290 | ]; |
||
291 | $users = $this->paginate($this->Users); |
||
292 | |||
293 | $showBottomNavigation = true; |
||
294 | |||
295 | $this->set(compact('menuItems', 'showBottomNavigation', 'users')); |
||
296 | } |
||
297 | |||
298 | /** |
||
299 | * Ignore user. |
||
300 | * |
||
301 | * @return void |
||
302 | */ |
||
303 | public function ignore() |
||
309 | |||
310 | /** |
||
311 | * Unignore user. |
||
312 | * |
||
313 | * @return void |
||
314 | */ |
||
315 | public function unignore() |
||
321 | |||
322 | /** |
||
323 | * Mark user as un-/ignored |
||
324 | * |
||
325 | * @param int $blockedId user to ignore |
||
326 | * @param bool $set block or unblock |
||
327 | * @return \Cake\Network\Response |
||
328 | */ |
||
329 | protected function _ignore($blockedId, $set) |
||
343 | |||
344 | /** |
||
345 | * Show user with profile $name |
||
346 | * |
||
347 | * @param string $name username |
||
348 | * @return void |
||
349 | */ |
||
350 | public function name($name = null) |
||
372 | |||
373 | /** |
||
374 | * View user profile. |
||
375 | * |
||
376 | * @param null $id user-ID |
||
377 | * @return \Cake\Network\Response|void |
||
378 | */ |
||
379 | public function view($id = null) |
||
380 | { |
||
381 | // redirect view/<username> to name/<username> |
||
382 | if (!empty($id) && !is_numeric($id)) { |
||
383 | $this->redirect( |
||
384 | ['controller' => 'users', 'action' => 'name', $id] |
||
385 | ); |
||
386 | |||
387 | return; |
||
388 | } |
||
389 | |||
390 | $id = (int)$id; |
||
391 | |||
392 | /** @var User */ |
||
393 | $user = $this->Users->find() |
||
394 | ->contain( |
||
395 | [ |
||
396 | 'UserBlocks' => function ($q) { |
||
397 | return $q->find('assocUsers'); |
||
398 | }, |
||
399 | 'UserOnline' |
||
400 | ] |
||
401 | ) |
||
402 | ->where(['Users.id' => (int)$id]) |
||
403 | ->first(); |
||
404 | |||
405 | if (empty($user)) { |
||
406 | $this->Flash->set(__('Invalid user'), ['element' => 'error']); |
||
407 | |||
408 | return $this->redirect('/'); |
||
409 | } |
||
410 | |||
411 | $entriesShownOnPage = 20; |
||
412 | $this->set( |
||
413 | 'lastEntries', |
||
414 | $this->Users->Entries->getRecentPostings( |
||
415 | $this->CurrentUser, |
||
416 | ['user_id' => $id, 'limit' => $entriesShownOnPage] |
||
417 | ) |
||
418 | ); |
||
419 | |||
420 | $this->set( |
||
421 | 'hasMoreEntriesThanShownOnPage', |
||
422 | ($user->numberOfPostings() - $entriesShownOnPage) > 0 |
||
423 | ); |
||
424 | |||
425 | if ($this->CurrentUser->getId() === $id) { |
||
426 | $ignores = $this->Users->UserIgnores->getAllIgnoredBy($id); |
||
427 | $user->set('ignores', $ignores); |
||
428 | } |
||
429 | |||
430 | $blockForm = new BlockForm(); |
||
431 | $solved = $this->Users->countSolved($id); |
||
432 | $this->set(compact('blockForm', 'isEditingAllowed', 'solved', 'user')); |
||
433 | $this->set('titleForLayout', $user->get('username')); |
||
434 | } |
||
435 | |||
436 | /** |
||
437 | * Set user avatar. |
||
438 | * |
||
439 | * @param string $userId user-ID |
||
440 | * @return void|\Cake\Network\Response |
||
441 | */ |
||
442 | public function avatar($userId) |
||
493 | |||
494 | /** |
||
495 | * Edit user. |
||
496 | * |
||
497 | * @param null $id user-ID |
||
498 | * |
||
499 | * @return \Cake\Network\Response|void |
||
500 | */ |
||
501 | public function edit($id = null) |
||
502 | { |
||
503 | /** @var User */ |
||
504 | $user = $this->Users->get($id); |
||
505 | |||
506 | $permissionEditing = $this->CurrentUser->permission( |
||
507 | 'saito.core.user.edit', |
||
508 | new Role($user->getRole()), |
||
509 | new Owner($user) |
||
510 | ); |
||
511 | if (!$permissionEditing) { |
||
512 | throw new \Saito\Exception\SaitoForbiddenException( |
||
513 | sprintf('Attempt to edit user "%s".', $user->get('id')), |
||
514 | ['CurrentUser' => $this->CurrentUser] |
||
515 | ); |
||
516 | } |
||
517 | |||
518 | if ($this->request->is('post') || $this->request->is('put')) { |
||
519 | $data = $this->request->getData(); |
||
520 | $patched = $this->Users->patchEntity($user, $data); |
||
521 | $errors = $patched->getErrors(); |
||
522 | if (empty($errors) && $this->Users->save($patched)) { |
||
523 | return $this->redirect(['action' => 'view', $id]); |
||
524 | } |
||
525 | |||
526 | $this->Flash->set( |
||
527 | __('The user could not be saved. Please, try again.'), |
||
528 | ['element' => 'error'] |
||
529 | ); |
||
530 | } |
||
531 | $this->set('user', $user); |
||
532 | |||
533 | $this->set( |
||
534 | 'titleForPage', |
||
535 | __('user.edit.t', [$user->get('username')]) |
||
536 | ); |
||
537 | |||
538 | $availableThemes = $this->Themes->getAvailable($this->CurrentUser); |
||
539 | $availableThemes = array_combine($availableThemes, $availableThemes); |
||
540 | $currentTheme = $this->Themes->getThemeForUser($this->CurrentUser); |
||
541 | $this->set(compact('availableThemes', 'currentTheme')); |
||
542 | } |
||
543 | |||
544 | /** |
||
545 | * delete user |
||
546 | * |
||
547 | * @param string $id user-ID |
||
548 | * @return \Cake\Network\Response|void |
||
549 | */ |
||
550 | public function delete($id) |
||
551 | { |
||
552 | $id = (int)$id; |
||
553 | /** @var User */ |
||
554 | $readUser = $this->Users->get($id); |
||
555 | |||
556 | /// Check permission |
||
557 | $permission = $this->CurrentUser->permission( |
||
558 | 'saito.core.user.delete', |
||
559 | new Role($readUser->getRole()) |
||
560 | ); |
||
561 | if (!$permission) { |
||
562 | throw new ForbiddenException( |
||
563 | sprintf( |
||
564 | 'User "%s" is not allowed to delete user "%s".', |
||
565 | $this->CurrentUser->get('username'), |
||
566 | $readUser->get('username') |
||
567 | ), |
||
568 | 1571811593 |
||
569 | ); |
||
570 | } |
||
571 | |||
572 | $this->set('user', $readUser); |
||
573 | |||
574 | $failure = false; |
||
575 | if (!$this->request->getData('userdeleteconfirm')) { |
||
576 | $failure = true; |
||
577 | $this->Flash->set(__('user.del.fail.3'), ['element' => 'error']); |
||
578 | } elseif ($this->CurrentUser->isUser($readUser)) { |
||
579 | $failure = true; |
||
580 | $this->Flash->set(__('user.del.fail.1'), ['element' => 'error']); |
||
581 | } |
||
582 | |||
583 | if (!$failure) { |
||
584 | $result = $this->Users->deleteAllExceptEntries($id); |
||
585 | if (empty($result)) { |
||
586 | $failure = true; |
||
587 | $this->Flash->set(__('user.del.fail.2'), ['element' => 'error']); |
||
588 | } |
||
589 | } |
||
590 | |||
591 | if ($failure) { |
||
592 | return $this->redirect( |
||
593 | [ |
||
594 | 'prefix' => false, |
||
595 | 'controller' => 'users', |
||
596 | 'action' => 'view', |
||
597 | $id |
||
598 | ] |
||
599 | ); |
||
600 | } |
||
601 | |||
602 | $this->Flash->set( |
||
603 | __('user.del.ok.m', $readUser->get('username')), |
||
604 | ['element' => 'success'] |
||
605 | ); |
||
606 | |||
607 | return $this->redirect('/'); |
||
608 | } |
||
609 | |||
610 | /** |
||
611 | * Lock user. |
||
612 | * |
||
613 | * @return \Cake\Network\Response|void |
||
614 | * @throws BadRequestException |
||
615 | */ |
||
616 | public function lock() |
||
657 | |||
658 | /** |
||
659 | * Unblock user. |
||
660 | * |
||
661 | * @param string $id user-ID |
||
662 | * @return void |
||
663 | */ |
||
664 | public function unlock(string $id) |
||
695 | |||
696 | /** |
||
697 | * changes user password |
||
698 | * |
||
699 | * @param null $id user-ID |
||
700 | * @return void |
||
701 | * @throws \Saito\Exception\SaitoForbiddenException |
||
702 | * @throws BadRequestException |
||
703 | */ |
||
704 | public function changepassword($id = null) |
||
760 | |||
761 | /** |
||
762 | * Directly set password for user |
||
763 | * |
||
764 | * @param string $id user-ID |
||
765 | * @return Response|null |
||
766 | */ |
||
767 | public function setpassword($id) |
||
801 | |||
802 | /** |
||
803 | * View and set user role |
||
804 | * |
||
805 | * @param string $id User-ID |
||
806 | * @return void|Response |
||
807 | */ |
||
808 | public function role($id) |
||
837 | |||
838 | /** |
||
839 | * Set slidetab-order. |
||
840 | * |
||
841 | * @return \Cake\Network\Response |
||
842 | * @throws BadRequestException |
||
843 | */ |
||
844 | public function slidetabOrder() |
||
875 | |||
876 | /** |
||
877 | * Shows user's uploads |
||
878 | * |
||
879 | * @return void |
||
880 | */ |
||
881 | public function uploads() |
||
884 | |||
885 | /** |
||
886 | * Set category for user. |
||
887 | * |
||
888 | * @param string|null $id category-ID |
||
889 | * @return \Cake\Network\Response |
||
890 | */ |
||
891 | public function setcategory(?string $id = null) |
||
905 | |||
906 | /** |
||
907 | * {@inheritdoc} |
||
908 | */ |
||
909 | public function beforeFilter(Event $event) |
||
930 | |||
931 | /** |
||
932 | * Logout user if logged in and create response to revisit logged out |
||
933 | * |
||
934 | * @return Response|null |
||
935 | */ |
||
936 | protected function _logoutAndComeHereAgain(): ?Response |
||
945 | } |
||
946 |
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.