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 MembersService 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 MembersService, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 43 | class MembersService { |
||
| 44 | |||
| 45 | /** @var string */ |
||
| 46 | private $userId; |
||
| 47 | |||
| 48 | /** @var IL10N */ |
||
| 49 | private $l10n; |
||
| 50 | |||
| 51 | /** @var IUserManager */ |
||
| 52 | private $userManager; |
||
| 53 | |||
| 54 | /** @var ConfigService */ |
||
| 55 | private $configService; |
||
| 56 | |||
| 57 | /** @var CirclesRequest */ |
||
| 58 | private $circlesRequest; |
||
| 59 | |||
| 60 | /** @var MembersRequest */ |
||
| 61 | private $membersRequest; |
||
| 62 | |||
| 63 | /** @var EventsService */ |
||
| 64 | private $eventsService; |
||
| 65 | |||
| 66 | /** @var MiscService */ |
||
| 67 | private $miscService; |
||
| 68 | |||
| 69 | /** |
||
| 70 | * MembersService constructor. |
||
| 71 | * |
||
| 72 | * @param $userId |
||
| 73 | * @param IL10N $l10n |
||
| 74 | * @param IUserManager $userManager |
||
| 75 | * @param ConfigService $configService |
||
| 76 | * @param CirclesRequest $circlesRequest |
||
| 77 | * @param MembersRequest $membersRequest |
||
| 78 | * @param EventsService $eventsService |
||
| 79 | * @param MiscService $miscService |
||
| 80 | */ |
||
| 81 | View Code Duplication | public function __construct( |
|
| 100 | |||
| 101 | |||
| 102 | /** |
||
| 103 | * @param string $circleUniqueId |
||
| 104 | * @param $ident |
||
| 105 | * @param $type |
||
| 106 | * |
||
| 107 | * @return array |
||
| 108 | * @throws \Exception |
||
| 109 | */ |
||
| 110 | public function addMember($circleUniqueId, $ident, $type) { |
||
| 136 | |||
| 137 | |||
| 138 | private function addMemberMassively(Circle $circle, $type, $ident) { |
||
| 146 | |||
| 147 | |||
| 148 | private function addMemberBasedOnItsType(Circle $circle, Member &$member) { |
||
| 153 | |||
| 154 | |||
| 155 | /** |
||
| 156 | * @param string $ident |
||
| 157 | * @param int $type |
||
| 158 | * |
||
| 159 | * @throws Exception |
||
| 160 | */ |
||
| 161 | private function verifyIdentBasedOnItsType(&$ident, $type) { |
||
| 169 | |||
| 170 | private function verifyIdentLocalMember(&$ident, $type) { |
||
| 181 | |||
| 182 | |||
| 183 | private function verifyIdentContact(&$ident, $type) { |
||
| 196 | |||
| 197 | |||
| 198 | /** |
||
| 199 | * @param Circle $circle |
||
| 200 | * @param Member $member |
||
| 201 | * |
||
| 202 | * @throws \Exception |
||
| 203 | */ |
||
| 204 | public function addLocalMember(Circle $circle, Member $member) { |
||
| 212 | |||
| 213 | |||
| 214 | /** |
||
| 215 | * @param Member $member |
||
| 216 | * |
||
| 217 | * @throws \Exception |
||
| 218 | */ |
||
| 219 | private function addEmailAddress(Member $member) { |
||
| 233 | |||
| 234 | |||
| 235 | /** |
||
| 236 | * @param Member $member |
||
| 237 | * |
||
| 238 | * @throws \Exception |
||
| 239 | */ |
||
| 240 | private function addContact(Member $member) { |
||
| 248 | |||
| 249 | |||
| 250 | /** |
||
| 251 | * @param Circle $circle |
||
| 252 | * @param string $groupId |
||
| 253 | * |
||
| 254 | * @return bool |
||
| 255 | * @throws \Exception |
||
| 256 | */ |
||
| 257 | private function addGroupMembers(Circle $circle, $groupId) { |
||
| 285 | |||
| 286 | |||
| 287 | /** |
||
| 288 | * getMember(); |
||
| 289 | * |
||
| 290 | * Will return any data of a user related to a circle (as a Member). User can be a 'non-member' |
||
| 291 | * Viewer needs to be at least Member of the Circle |
||
| 292 | * |
||
| 293 | * @param $circleId |
||
| 294 | * @param $userId |
||
| 295 | * @param $type |
||
| 296 | * |
||
| 297 | * @return Member |
||
| 298 | * @throws \Exception |
||
| 299 | */ |
||
| 300 | public function getMember($circleId, $userId, $type) { |
||
| 315 | |||
| 316 | |||
| 317 | /** |
||
| 318 | * @param string $circleUniqueId |
||
| 319 | * @param string $name |
||
| 320 | * @param int $type |
||
| 321 | * @param int $level |
||
| 322 | * |
||
| 323 | * @return array |
||
| 324 | * @throws \Exception |
||
| 325 | */ |
||
| 326 | public function levelMember($circleUniqueId, $name, $type, $level) { |
||
| 357 | |||
| 358 | |||
| 359 | /** |
||
| 360 | * @param Circle $circle |
||
| 361 | * @param Member $member |
||
| 362 | * @param $level |
||
| 363 | * |
||
| 364 | * @throws \Exception |
||
| 365 | */ |
||
| 366 | private function editMemberLevel(Circle $circle, Member &$member, $level) { |
||
| 367 | try { |
||
| 368 | $isMod = $circle->getHigherViewer(); |
||
| 369 | $isMod->hasToBeModerator(); |
||
| 370 | $isMod->hasToBeHigherLevel($level); |
||
| 371 | |||
| 372 | $member->hasToBeMember(); |
||
| 373 | $member->cantBeOwner(); |
||
| 374 | $isMod->hasToBeHigherLevel($member->getLevel()); |
||
| 375 | |||
| 376 | $member->setLevel($level); |
||
| 377 | $this->membersRequest->updateMember($member); |
||
| 378 | } catch (\Exception $e) { |
||
| 379 | throw $e; |
||
| 380 | } |
||
| 381 | |||
| 382 | } |
||
| 383 | |||
| 384 | /** |
||
| 385 | * @param Circle $circle |
||
| 386 | * @param Member $member |
||
| 387 | * |
||
| 388 | * @throws \Exception |
||
| 389 | */ |
||
| 390 | private function switchOwner(Circle $circle, Member &$member) { |
||
| 408 | |||
| 409 | |||
| 410 | /** |
||
| 411 | * @param string $circleUniqueId |
||
| 412 | * @param string $name |
||
| 413 | * @param $type |
||
| 414 | * |
||
| 415 | * @return array |
||
| 416 | * @throws \Exception |
||
| 417 | */ |
||
| 418 | public function removeMember($circleUniqueId, $name, $type) { |
||
| 445 | |||
| 446 | |||
| 447 | /** |
||
| 448 | * When a user is removed, remove him from all Circles |
||
| 449 | * |
||
| 450 | * @param $userId |
||
| 451 | */ |
||
| 452 | public function onUserRemoved($userId) { |
||
| 455 | |||
| 456 | |||
| 457 | } |
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.