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 |
||
22 | class User extends DataObject |
||
23 | { |
||
24 | const STATUS_ACTIVE = 'Active'; |
||
25 | const STATUS_SUSPENDED = 'Suspended'; |
||
26 | const STATUS_DECLINED = 'Declined'; |
||
27 | const STATUS_NEW = 'New'; |
||
28 | const CREATION_MANUAL = 0; |
||
29 | const CREATION_OAUTH = 1; |
||
30 | const CREATION_BOT = 2; |
||
31 | private $username; |
||
32 | private $email; |
||
33 | private $status = self::STATUS_NEW; |
||
34 | private $onwikiname; |
||
35 | private $welcome_sig = ""; |
||
36 | private $lastactive = "0000-00-00 00:00:00"; |
||
37 | private $forcelogout = 0; |
||
38 | private $forceidentified = null; |
||
39 | private $welcome_template = 0; |
||
40 | private $abortpref = 0; |
||
41 | private $confirmationdiff = 0; |
||
42 | private $emailsig = ""; |
||
43 | private $creationmode = 0; |
||
44 | /** @var User Cache variable of the current user - it's never going to change in the middle of a request. */ |
||
45 | private static $currentUser; |
||
46 | #region Object load methods |
||
47 | |||
48 | /** |
||
49 | * Gets the currently logged in user |
||
50 | * |
||
51 | * @param PdoDatabase $database |
||
52 | * |
||
53 | * @return User|CommunityUser |
||
54 | */ |
||
55 | public static function getCurrent(PdoDatabase $database) |
||
80 | |||
81 | /** |
||
82 | * Gets a user by their user ID |
||
83 | * |
||
84 | * Pass -1 to get the community user. |
||
85 | * |
||
86 | * @param int|null $id |
||
87 | * @param PdoDatabase $database |
||
88 | * |
||
89 | * @return User|false |
||
90 | */ |
||
91 | public static function getById($id, PdoDatabase $database) |
||
102 | |||
103 | /** |
||
104 | * @return CommunityUser |
||
105 | */ |
||
106 | public static function getCommunity() |
||
110 | |||
111 | /** |
||
112 | * Gets a user by their username |
||
113 | * |
||
114 | * @param string $username |
||
115 | * @param PdoDatabase $database |
||
116 | * |
||
117 | * @return CommunityUser|User|false |
||
118 | */ |
||
119 | public static function getByUsername($username, PdoDatabase $database) |
||
139 | |||
140 | /** |
||
141 | * Gets a user by their on-wiki username. |
||
142 | * |
||
143 | * @param string $username |
||
144 | * @param PdoDatabase $database |
||
145 | * |
||
146 | * @return User|false |
||
147 | */ |
||
148 | View Code Duplication | public static function getByOnWikiUsername($username, PdoDatabase $database) |
|
|
|||
149 | { |
||
150 | $statement = $database->prepare("SELECT * FROM user WHERE onwikiname = :id LIMIT 1;"); |
||
151 | $statement->bindValue(":id", $username); |
||
152 | $statement->execute(); |
||
153 | |||
154 | $resultObject = $statement->fetchObject(get_called_class()); |
||
155 | |||
156 | if ($resultObject != false) { |
||
157 | $resultObject->setDatabase($database); |
||
158 | |||
159 | return $resultObject; |
||
160 | } |
||
161 | |||
162 | return false; |
||
163 | } |
||
164 | |||
165 | #endregion |
||
166 | |||
167 | /** |
||
168 | * Saves the current object |
||
169 | * |
||
170 | * @throws Exception |
||
171 | */ |
||
172 | public function save() |
||
255 | |||
256 | #region properties |
||
257 | |||
258 | /** |
||
259 | * Gets the tool username |
||
260 | * @return string |
||
261 | */ |
||
262 | public function getUsername() |
||
266 | |||
267 | /** |
||
268 | * Sets the tool username |
||
269 | * |
||
270 | * @param string $username |
||
271 | */ |
||
272 | public function setUsername($username) |
||
281 | |||
282 | /** |
||
283 | * Gets the user's email address |
||
284 | * @return string |
||
285 | */ |
||
286 | public function getEmail() |
||
290 | |||
291 | /** |
||
292 | * Sets the user's email address |
||
293 | * |
||
294 | * @param string $email |
||
295 | */ |
||
296 | public function setEmail($email) |
||
300 | |||
301 | /** |
||
302 | * Gets the status (User, Admin, Suspended, etc - excludes checkuser) of the user. |
||
303 | * @return string |
||
304 | */ |
||
305 | public function getStatus() |
||
309 | |||
310 | /** |
||
311 | * @param string $status |
||
312 | */ |
||
313 | public function setStatus($status) |
||
317 | |||
318 | /** |
||
319 | * Gets the user's on-wiki name |
||
320 | * @return string |
||
321 | */ |
||
322 | public function getOnWikiName() |
||
326 | |||
327 | /** |
||
328 | * Sets the user's on-wiki name |
||
329 | * |
||
330 | * This can have interesting side-effects with OAuth. |
||
331 | * |
||
332 | * @param string $onWikiName |
||
333 | */ |
||
334 | public function setOnWikiName($onWikiName) |
||
338 | |||
339 | /** |
||
340 | * Gets the welcome signature |
||
341 | * @return string |
||
342 | */ |
||
343 | public function getWelcomeSig() |
||
347 | |||
348 | /** |
||
349 | * Sets the welcome signature |
||
350 | * |
||
351 | * @param string $welcomeSig |
||
352 | */ |
||
353 | public function setWelcomeSig($welcomeSig) |
||
357 | |||
358 | /** |
||
359 | * Gets the last activity date for the user |
||
360 | * |
||
361 | * @return string |
||
362 | * @todo This should probably return an instance of DateTime |
||
363 | */ |
||
364 | public function getLastActive() |
||
368 | |||
369 | /** |
||
370 | * Gets the user's forced logout status |
||
371 | * |
||
372 | * @return bool |
||
373 | */ |
||
374 | public function getForceLogout() |
||
378 | |||
379 | /** |
||
380 | * Sets the user's forced logout status |
||
381 | * |
||
382 | * @param bool $forceLogout |
||
383 | */ |
||
384 | public function setForceLogout($forceLogout) |
||
388 | |||
389 | /** |
||
390 | * Returns the ID of the welcome template used. |
||
391 | * @return int |
||
392 | */ |
||
393 | public function getWelcomeTemplate() |
||
397 | |||
398 | /** |
||
399 | * Sets the ID of the welcome template used. |
||
400 | * |
||
401 | * @param int $welcomeTemplate |
||
402 | */ |
||
403 | public function setWelcomeTemplate($welcomeTemplate) |
||
407 | |||
408 | /** |
||
409 | * Gets the user's abort preference |
||
410 | * @todo this is badly named too! Also a bool that's actually an int. |
||
411 | * @return int |
||
412 | */ |
||
413 | public function getAbortPref() |
||
417 | |||
418 | /** |
||
419 | * Sets the user's abort preference |
||
420 | * @todo rename, retype, and re-comment. |
||
421 | * |
||
422 | * @param int $abortPreference |
||
423 | */ |
||
424 | public function setAbortPref($abortPreference) |
||
428 | |||
429 | /** |
||
430 | * Gets the user's confirmation diff. Unused if OAuth is in use. |
||
431 | * @return int the diff ID |
||
432 | */ |
||
433 | public function getConfirmationDiff() |
||
437 | |||
438 | /** |
||
439 | * Sets the user's confirmation diff. |
||
440 | * |
||
441 | * @param int $confirmationDiff |
||
442 | */ |
||
443 | public function setConfirmationDiff($confirmationDiff) |
||
447 | |||
448 | /** |
||
449 | * Gets the users' email signature used on outbound mail. |
||
450 | * @todo rename me! |
||
451 | * @return string |
||
452 | */ |
||
453 | public function getEmailSig() |
||
457 | |||
458 | /** |
||
459 | * Sets the user's email signature for outbound mail. |
||
460 | * |
||
461 | * @param string $emailSignature |
||
462 | */ |
||
463 | public function setEmailSig($emailSignature) |
||
467 | |||
468 | /** |
||
469 | * @return int |
||
470 | */ |
||
471 | public function getCreationMode() |
||
475 | |||
476 | /** |
||
477 | * @param $creationMode int |
||
478 | */ |
||
479 | public function setCreationMode($creationMode) |
||
483 | |||
484 | #endregion |
||
485 | |||
486 | #region user access checks |
||
487 | |||
488 | public function isActive() |
||
492 | |||
493 | /** |
||
494 | * Tests if the user is identified |
||
495 | * |
||
496 | * @param IdentificationVerifier $iv |
||
497 | * |
||
498 | * @return bool |
||
499 | * @todo Figure out what on earth is going on with PDO's typecasting here. Apparently, it returns string("0") for |
||
500 | * the force-unidentified case, and int(1) for the identified case?! This is quite ugly, but probably needed |
||
501 | * to play it safe for now. |
||
502 | * @category Security-Critical |
||
503 | */ |
||
504 | public function isIdentified(IdentificationVerifier $iv) |
||
519 | |||
520 | /** |
||
521 | * Tests if the user is suspended |
||
522 | * @return bool |
||
523 | * @category Security-Critical |
||
524 | */ |
||
525 | public function isSuspended() |
||
529 | |||
530 | /** |
||
531 | * Tests if the user is new |
||
532 | * @return bool |
||
533 | * @category Security-Critical |
||
534 | */ |
||
535 | public function isNewUser() |
||
539 | |||
540 | /** |
||
541 | * Tests if the user has been declined access to the tool |
||
542 | * @return bool |
||
543 | * @category Security-Critical |
||
544 | */ |
||
545 | public function isDeclined() |
||
549 | |||
550 | /** |
||
551 | * Tests if the user is the community user |
||
552 | * |
||
553 | * @todo decide if this means logged out. I think it usually does. |
||
554 | * @return bool |
||
555 | * @category Security-Critical |
||
556 | */ |
||
557 | public function isCommunityUser() |
||
561 | |||
562 | #endregion |
||
563 | |||
564 | /** |
||
565 | * Gets a hash of data for the user to reset their password with. |
||
566 | * @category Security-Critical |
||
567 | * @return string |
||
568 | */ |
||
569 | public function getForgottenPasswordHash() |
||
574 | |||
575 | /** |
||
576 | * Gets the approval date of the user |
||
577 | * @return DateTime|false |
||
578 | */ |
||
579 | public function getApprovalDate() |
||
598 | } |
||
599 |
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.