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 User 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 User, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
28 | class User extends DataObject |
||
29 | { |
||
30 | const STATUS_ACTIVE = 'Active'; |
||
31 | const STATUS_SUSPENDED = 'Suspended'; |
||
32 | const STATUS_DECLINED = 'Declined'; |
||
33 | const STATUS_NEW = 'New'; |
||
34 | private $username; |
||
35 | private $email; |
||
36 | private $password; |
||
37 | private $status = self::STATUS_NEW; |
||
38 | private $onwikiname = "##OAUTH##"; |
||
39 | private $welcome_sig = ""; |
||
40 | private $lastactive = "0000-00-00 00:00:00"; |
||
41 | private $forcelogout = 0; |
||
42 | private $forceidentified = null; |
||
43 | private $welcome_template = 0; |
||
44 | private $abortpref = 0; |
||
45 | private $confirmationdiff = 0; |
||
46 | private $emailsig = ""; |
||
47 | /** @var null|string */ |
||
48 | private $oauthrequesttoken = null; |
||
49 | /** @var null|string */ |
||
50 | private $oauthrequestsecret = null; |
||
51 | /** @var null|string */ |
||
52 | private $oauthaccesstoken = null; |
||
53 | /** @var null|string */ |
||
54 | private $oauthaccesssecret = null; |
||
55 | private $oauthidentitycache = null; |
||
56 | /** @var User Cache variable of the current user - it's never going to change in the middle of a request. */ |
||
57 | private static $currentUser; |
||
58 | /** @var null|JWT The identity cache */ |
||
59 | private $identityCache = null; |
||
60 | #region Object load methods |
||
61 | |||
62 | /** |
||
63 | * Gets the currently logged in user |
||
64 | * |
||
65 | * @param PdoDatabase $database |
||
66 | * |
||
67 | * @return User|CommunityUser |
||
68 | */ |
||
69 | public static function getCurrent(PdoDatabase $database) |
||
70 | { |
||
71 | if (self::$currentUser === null) { |
||
72 | $sessionId = WebRequest::getSessionUserId(); |
||
73 | |||
74 | if ($sessionId !== null) { |
||
75 | /** @var User $user */ |
||
76 | $user = self::getById($sessionId, $database); |
||
77 | |||
78 | if ($user === false) { |
||
79 | self::$currentUser = new CommunityUser(); |
||
80 | } |
||
81 | else { |
||
82 | self::$currentUser = $user; |
||
83 | } |
||
84 | } |
||
85 | else { |
||
86 | $anonymousCoward = new CommunityUser(); |
||
87 | |||
88 | self::$currentUser = $anonymousCoward; |
||
89 | } |
||
90 | } |
||
91 | |||
92 | return self::$currentUser; |
||
93 | } |
||
94 | |||
95 | /** |
||
96 | * Gets a user by their user ID |
||
97 | * |
||
98 | * Pass -1 to get the community user. |
||
99 | * |
||
100 | * @param int|null $id |
||
101 | * @param PdoDatabase $database |
||
102 | * |
||
103 | * @return User|false |
||
104 | */ |
||
105 | public static function getById($id, PdoDatabase $database) |
||
106 | { |
||
107 | if ($id === null || $id == -1) { |
||
108 | return new CommunityUser(); |
||
109 | } |
||
110 | |||
111 | /** @var User|false $user */ |
||
112 | $user = parent::getById($id, $database); |
||
113 | |||
114 | return $user; |
||
115 | } |
||
116 | |||
117 | /** |
||
118 | * @return CommunityUser |
||
119 | */ |
||
120 | 2 | public static function getCommunity() |
|
124 | |||
125 | /** |
||
126 | * Gets a user by their username |
||
127 | * |
||
128 | * @param string $username |
||
129 | * @param PdoDatabase $database |
||
130 | * |
||
131 | * @return CommunityUser|User|false |
||
132 | */ |
||
133 | public static function getByUsername($username, PdoDatabase $database) |
||
153 | |||
154 | /** |
||
155 | * Gets a user by their on-wiki username. |
||
156 | * |
||
157 | * Don't use without asking me first. It's really inefficient in it's current implementation. |
||
158 | * We need to restructure the user table again to make this more efficient. |
||
159 | * We don't actually store the on-wiki name in the table any more, instead we |
||
160 | * are storing JSON in a column (!!). Yep, my fault. Code review is an awesome thing. |
||
161 | * -- stw 2015-10-20 |
||
162 | * |
||
163 | * @param string $username |
||
164 | * @param PdoDatabase $database |
||
165 | * |
||
166 | * @return User|false |
||
167 | */ |
||
168 | public static function getByOnWikiUsername($username, PdoDatabase $database) |
||
216 | |||
217 | /** |
||
218 | * Gets a user by their OAuth request token |
||
219 | * |
||
220 | * @param string $requestToken |
||
221 | * @param PdoDatabase $database |
||
222 | * |
||
223 | * @return User|false |
||
224 | */ |
||
225 | public static function getByRequestToken($requestToken, PdoDatabase $database) |
||
240 | |||
241 | #endregion |
||
242 | |||
243 | /** |
||
244 | * Saves the current object |
||
245 | * |
||
246 | * @throws Exception |
||
247 | */ |
||
248 | public function save() |
||
344 | |||
345 | /** |
||
346 | * Authenticates the user with the supplied password |
||
347 | * |
||
348 | * @param string $password |
||
349 | * |
||
350 | * @return bool |
||
351 | * @throws Exception |
||
352 | * @category Security-Critical |
||
353 | */ |
||
354 | public function authenticate($password) |
||
368 | |||
369 | #region properties |
||
370 | |||
371 | /** |
||
372 | * Gets the tool username |
||
373 | * @return string |
||
374 | */ |
||
375 | public function getUsername() |
||
379 | |||
380 | /** |
||
381 | * Sets the tool username |
||
382 | * |
||
383 | * @param string $username |
||
384 | */ |
||
385 | public function setUsername($username) |
||
394 | |||
395 | /** |
||
396 | * Gets the user's email address |
||
397 | * @return string |
||
398 | */ |
||
399 | public function getEmail() |
||
403 | |||
404 | /** |
||
405 | * Sets the user's email address |
||
406 | * |
||
407 | * @param string $email |
||
408 | */ |
||
409 | public function setEmail($email) |
||
413 | |||
414 | /** |
||
415 | * Sets the user's password |
||
416 | * |
||
417 | * @param string $password the plaintext password |
||
418 | * |
||
419 | * @category Security-Critical |
||
420 | */ |
||
421 | public function setPassword($password) |
||
425 | |||
426 | /** |
||
427 | * Gets the status (User, Admin, Suspended, etc - excludes checkuser) of the user. |
||
428 | * @return string |
||
429 | */ |
||
430 | public function getStatus() |
||
434 | |||
435 | /** |
||
436 | * @param string $status |
||
437 | */ |
||
438 | public function setStatus($status) |
||
442 | |||
443 | /** |
||
444 | * Gets the user's on-wiki name |
||
445 | * @return string |
||
446 | */ |
||
447 | public function getOnWikiName() |
||
461 | |||
462 | /** |
||
463 | * This is probably NOT the function you want! |
||
464 | * |
||
465 | * Take a look at getOnWikiName() instead. |
||
466 | * @return string |
||
467 | */ |
||
468 | public function getStoredOnWikiName() |
||
472 | |||
473 | /** |
||
474 | * Sets the user's on-wiki name |
||
475 | * |
||
476 | * This can have interesting side-effects with OAuth. |
||
477 | * |
||
478 | * @param string $onWikiName |
||
479 | */ |
||
480 | public function setOnWikiName($onWikiName) |
||
484 | |||
485 | /** |
||
486 | * Gets the welcome signature |
||
487 | * @return string |
||
488 | */ |
||
489 | public function getWelcomeSig() |
||
493 | |||
494 | /** |
||
495 | * Sets the welcome signature |
||
496 | * |
||
497 | * @param string $welcomeSig |
||
498 | */ |
||
499 | public function setWelcomeSig($welcomeSig) |
||
503 | |||
504 | /** |
||
505 | * Gets the last activity date for the user |
||
506 | * |
||
507 | * @return string |
||
508 | * @todo This should probably return an instance of DateTime |
||
|
|||
509 | */ |
||
510 | public function getLastActive() |
||
514 | |||
515 | /** |
||
516 | * Gets the user's forced logout status |
||
517 | * |
||
518 | * @return bool |
||
519 | */ |
||
520 | public function getForceLogout() |
||
524 | |||
525 | /** |
||
526 | * Sets the user's forced logout status |
||
527 | * |
||
528 | * @param bool $forceLogout |
||
529 | */ |
||
530 | public function setForceLogout($forceLogout) |
||
534 | |||
535 | /** |
||
536 | * Returns the ID of the welcome template used. |
||
537 | * @return int |
||
538 | */ |
||
539 | public function getWelcomeTemplate() |
||
543 | |||
544 | /** |
||
545 | * Sets the ID of the welcome template used. |
||
546 | * |
||
547 | * @param int $welcomeTemplate |
||
548 | */ |
||
549 | public function setWelcomeTemplate($welcomeTemplate) |
||
553 | |||
554 | /** |
||
555 | * Gets the user's abort preference |
||
556 | * @todo this is badly named too! Also a bool that's actually an int. |
||
557 | * @return int |
||
558 | */ |
||
559 | public function getAbortPref() |
||
563 | |||
564 | /** |
||
565 | * Sets the user's abort preference |
||
566 | * @todo rename, retype, and re-comment. |
||
567 | * |
||
568 | * @param int $abortPreference |
||
569 | */ |
||
570 | public function setAbortPref($abortPreference) |
||
574 | |||
575 | /** |
||
576 | * Gets the user's confirmation diff. Unused if OAuth is in use. |
||
577 | * @return int the diff ID |
||
578 | */ |
||
579 | public function getConfirmationDiff() |
||
583 | |||
584 | /** |
||
585 | * Sets the user's confirmation diff. |
||
586 | * |
||
587 | * @param int $confirmationDiff |
||
588 | */ |
||
589 | public function setConfirmationDiff($confirmationDiff) |
||
593 | |||
594 | /** |
||
595 | * Gets the users' email signature used on outbound mail. |
||
596 | * @todo rename me! |
||
597 | * @return string |
||
598 | */ |
||
599 | public function getEmailSig() |
||
603 | |||
604 | /** |
||
605 | * Sets the user's email signature for outbound mail. |
||
606 | * |
||
607 | * @param string $emailSignature |
||
608 | */ |
||
609 | public function setEmailSig($emailSignature) |
||
613 | |||
614 | /** |
||
615 | * Gets the user's OAuth request token. |
||
616 | * |
||
617 | * @todo move me to a collaborator. |
||
618 | * @return null|string |
||
619 | */ |
||
620 | public function getOAuthRequestToken() |
||
624 | |||
625 | /** |
||
626 | * Sets the user's OAuth request token |
||
627 | * @todo move me to a collaborator |
||
628 | * |
||
629 | * @param string $oAuthRequestToken |
||
630 | */ |
||
631 | public function setOAuthRequestToken($oAuthRequestToken) |
||
635 | |||
636 | /** |
||
637 | * Gets the users OAuth request secret |
||
638 | * @category Security-Critical |
||
639 | * @todo move me to a collaborator |
||
640 | * @return null|string |
||
641 | */ |
||
642 | public function getOAuthRequestSecret() |
||
646 | |||
647 | /** |
||
648 | * Sets the user's OAuth request secret |
||
649 | * @todo move me to a collaborator |
||
650 | * |
||
651 | * @param string $oAuthRequestSecret |
||
652 | */ |
||
653 | public function setOAuthRequestSecret($oAuthRequestSecret) |
||
657 | |||
658 | /** |
||
659 | * Gets the user's access token |
||
660 | * @category Security-Critical |
||
661 | * @todo move me to a collaborator |
||
662 | * @return null|string |
||
663 | */ |
||
664 | public function getOAuthAccessToken() |
||
668 | |||
669 | /** |
||
670 | * Sets the user's access token |
||
671 | * @todo move me to a collaborator |
||
672 | * |
||
673 | * @param string $oAuthAccessToken |
||
674 | */ |
||
675 | public function setOAuthAccessToken($oAuthAccessToken) |
||
679 | |||
680 | /** |
||
681 | * Gets the user's OAuth access secret |
||
682 | * @category Security-Critical |
||
683 | * @todo move me to a collaborator |
||
684 | * @return null|string |
||
685 | */ |
||
686 | public function getOAuthAccessSecret() |
||
690 | |||
691 | /** |
||
692 | * Sets the user's OAuth access secret |
||
693 | * @todo move me to a collaborator |
||
694 | * |
||
695 | * @param string $oAuthAccessSecret |
||
696 | */ |
||
697 | public function setOAuthAccessSecret($oAuthAccessSecret) |
||
701 | |||
702 | #endregion |
||
703 | |||
704 | #region user access checks |
||
705 | |||
706 | public function isActive() |
||
710 | |||
711 | /** |
||
712 | * Tests if the user is identified |
||
713 | * |
||
714 | * @param IdentificationVerifier $iv |
||
715 | * |
||
716 | * @return bool |
||
717 | * @todo Figure out what on earth is going on with PDO's typecasting here. Apparently, it returns string("0") for |
||
718 | * the force-unidentified case, and int(1) for the identified case?! This is quite ugly, but probably needed |
||
719 | * to play it safe for now. |
||
720 | * @category Security-Critical |
||
721 | */ |
||
722 | public function isIdentified(IdentificationVerifier $iv) |
||
737 | |||
738 | /** |
||
739 | * Tests if the user is suspended |
||
740 | * @return bool |
||
741 | * @category Security-Critical |
||
742 | */ |
||
743 | public function isSuspended() |
||
747 | |||
748 | /** |
||
749 | * Tests if the user is new |
||
750 | * @return bool |
||
751 | * @category Security-Critical |
||
752 | */ |
||
753 | public function isNewUser() |
||
757 | |||
758 | /** |
||
759 | * Tests if the user has been declined access to the tool |
||
760 | * @return bool |
||
761 | * @category Security-Critical |
||
762 | */ |
||
763 | public function isDeclined() |
||
767 | |||
768 | /** |
||
769 | * Tests if the user is the community user |
||
770 | * |
||
771 | * @todo decide if this means logged out. I think it usually does. |
||
772 | * @return bool |
||
773 | * @category Security-Critical |
||
774 | */ |
||
775 | public function isCommunityUser() |
||
779 | |||
780 | #endregion |
||
781 | |||
782 | #region OAuth |
||
783 | |||
784 | /** |
||
785 | * @todo move me to a collaborator |
||
786 | * |
||
787 | * @param bool $useCached |
||
788 | * |
||
789 | * @return mixed|null |
||
790 | * @category Security-Critical |
||
791 | */ |
||
792 | public function getOAuthIdentity($useCached = false) |
||
836 | |||
837 | /** |
||
838 | * @todo move me to a collaborator |
||
839 | * |
||
840 | * @param mixed $useCached Set to false for everything where up-to-date data is important. |
||
841 | * |
||
842 | * @return mixed |
||
843 | * @category Security-Critical |
||
844 | */ |
||
845 | private function getOAuthOnWikiName($useCached = false) |
||
854 | |||
855 | /** |
||
856 | * @return bool |
||
857 | * @todo move me to a collaborator |
||
858 | */ |
||
859 | public function isOAuthLinked() |
||
867 | |||
868 | /** |
||
869 | * @return null |
||
870 | * @todo move me to a collaborator |
||
871 | */ |
||
872 | public function clearOAuthData() |
||
881 | |||
882 | /** |
||
883 | * @throws Exception |
||
884 | * @todo move me to a collaborator |
||
885 | * @category Security-Critical |
||
886 | */ |
||
887 | private function getIdentityCache() |
||
908 | |||
909 | /** |
||
910 | * @return bool |
||
911 | * @todo move me to a collaborator |
||
912 | */ |
||
913 | public function oauthCanUse() |
||
922 | |||
923 | /** |
||
924 | * @return bool |
||
925 | * @todo move me to a collaborator |
||
926 | */ |
||
927 | View Code Duplication | public function oauthCanEdit() |
|
940 | |||
941 | /** |
||
942 | * @return bool |
||
943 | * @todo move me to a collaborator |
||
944 | */ |
||
945 | View Code Duplication | public function oauthCanCreateAccount() |
|
957 | |||
958 | /** |
||
959 | * @return bool |
||
960 | * @todo move me to a collaborator |
||
961 | * @category Security-Critical |
||
962 | */ |
||
963 | protected function oauthCanCheckUser() |
||
978 | |||
979 | #endregion |
||
980 | |||
981 | /** |
||
982 | * Gets a hash of data for the user to reset their password with. |
||
983 | * @category Security-Critical |
||
984 | * @return string |
||
985 | */ |
||
986 | public function getForgottenPasswordHash() |
||
990 | |||
991 | /** |
||
992 | * Gets the approval date of the user |
||
993 | * @return DateTime|false |
||
994 | */ |
||
995 | public function getApprovalDate() |
||
1014 | } |
||
1015 |
This check looks
TODO
comments that have been left in the code.``TODO``s show that something is left unfinished and should be attended to.