the-badger /
badger
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 DefaultController 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 DefaultController, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 14 | class DefaultController extends Controller |
||
| 15 | { |
||
| 16 | /** |
||
| 17 | * @Route("/", name="homepage") |
||
| 18 | */ |
||
| 19 | public function indexAction() |
||
| 20 | { |
||
| 21 | $mostUnlockedBadges = []; |
||
| 22 | $badgeChampions = []; |
||
| 23 | $userBadgeCompletions = []; |
||
| 24 | $userTags = $this->getUser()->getTags()->toArray(); |
||
| 25 | |||
| 26 | // Put default tag first |
||
| 27 | usort($userTags, function ($a, $b) |
||
| 28 | { |
||
| 29 | if ($a->isDefault()) { |
||
| 30 | return -1; |
||
| 31 | } else if($b->isDefault()) { |
||
| 32 | return 1; |
||
| 33 | } |
||
| 34 | }); |
||
| 35 | |||
| 36 | // Get most unlocked badges & champions per tag |
||
| 37 | foreach ($userTags as $tag) { |
||
| 38 | $month = date('m'); |
||
|
|
|||
| 39 | $month = "06"; |
||
| 40 | $year = date('Y'); |
||
| 41 | $year = "2016"; |
||
| 42 | $currentUserIsChampion = false; |
||
| 43 | |||
| 44 | $mostUnlockedBadges[$tag->getCode()] = $this->get('badger.game.repository.badge_completion') |
||
| 45 | ->getMostUnlockedBadgesForMonth($month, $year, $tag, 5); |
||
| 46 | |||
| 47 | $maxBadgeCompletions = $this->get('badger.game.repository.badge_completion') |
||
| 48 | ->getTopNumberOfUnlocksForMonth($month, $year, $tag); |
||
| 49 | |||
| 50 | $champions = $this->get('badger.user.repository.user') |
||
| 51 | ->getMonthlyBadgeChampions($month, $year, $tag, $maxBadgeCompletions); |
||
| 52 | |||
| 53 | foreach ($champions as $champion) { |
||
| 54 | if ($champion['user']->getId() === $this->getUser()->getId()) { |
||
| 55 | $currentUserIsChampion = true; |
||
| 56 | } |
||
| 57 | |||
| 58 | $badgeChampions[$tag->getCode()][$champion['badgeCompletions']][] = $champion['user']; |
||
| 59 | } |
||
| 60 | |||
| 61 | if (!$currentUserIsChampion) { |
||
| 62 | $userCompletions = $this->get('badger.game.repository.badge_completion') |
||
| 63 | ->getTopNumberOfUnlocksForMonth($month, $year, $tag, $this->getUser()); |
||
| 64 | |||
| 65 | if (!empty($userCompletions)) { |
||
| 66 | $userBadgeCompletions[$tag->getCode()] = current($userCompletions)['nbCompletions']; |
||
| 67 | } |
||
| 68 | } |
||
| 69 | |||
| 70 | } |
||
| 71 | |||
| 72 | $newMembers = $this->get('badger.user.repository.user')->getNewUsersForMonth($month, $year); |
||
| 73 | |||
| 74 | return $this->render('@Game/home.html.twig', [ |
||
| 75 | 'newMembers' => $newMembers, |
||
| 76 | 'mostUnlockedBadges' => $mostUnlockedBadges, |
||
| 77 | 'userTags' => $userTags, |
||
| 78 | 'badgeChampions' => $badgeChampions, |
||
| 79 | 'userBadgeCompletions' => $userBadgeCompletions |
||
| 80 | ]); |
||
| 81 | } |
||
| 82 | |||
| 83 | /** |
||
| 84 | * @Route("/users", name="users") |
||
| 85 | */ |
||
| 86 | public function usersAction() |
||
| 94 | |||
| 95 | /** |
||
| 96 | * @Route("/admin", name="admin") |
||
| 97 | */ |
||
| 98 | public function adminAction() |
||
| 102 | |||
| 103 | /** |
||
| 104 | * @Route("/user/{username}", name="userprofile") |
||
| 105 | * @param string $username |
||
| 106 | * |
||
| 107 | * @return Response |
||
| 108 | */ |
||
| 109 | public function userProfileAction($username) |
||
| 142 | |||
| 143 | /** |
||
| 144 | * @Route("/badge/{id}", name="viewbadge") |
||
| 145 | * @param $id |
||
| 146 | * |
||
| 147 | * @return Response |
||
| 148 | */ |
||
| 149 | public function badgeViewAction($id) |
||
| 184 | |||
| 185 | /** |
||
| 186 | * @Route("/claim/badge/{id}", name="claimbadge") |
||
| 187 | * @param int $id |
||
| 188 | * |
||
| 189 | * @return JsonResponse |
||
| 190 | */ |
||
| 191 | public function claimBadgeAction($id) |
||
| 220 | |||
| 221 | /** |
||
| 222 | * @Route("/claim/step/{id}", name="claimstep") |
||
| 223 | * @param int $id |
||
| 224 | * |
||
| 225 | * @return JsonResponse |
||
| 226 | */ |
||
| 227 | View Code Duplication | public function claimStepAction($id) |
|
| 256 | |||
| 257 | /** |
||
| 258 | * @Route("/claim/quest/{id}", name="claimquest") |
||
| 259 | * @param int $id |
||
| 260 | * |
||
| 261 | * @return JsonResponse |
||
| 262 | */ |
||
| 263 | View Code Duplication | public function claimQuestAction($id) |
|
| 292 | |||
| 293 | /** |
||
| 294 | * @Route("/badges", name="badges") |
||
| 295 | * |
||
| 296 | * @return Response |
||
| 297 | */ |
||
| 298 | public function badgeListAction() |
||
| 315 | |||
| 316 | /** |
||
| 317 | * @Route("/quests", name="quests") |
||
| 318 | * @param Request $request |
||
| 319 | * |
||
| 320 | * @return Response |
||
| 321 | */ |
||
| 322 | public function questListAction(Request $request) |
||
| 344 | |||
| 345 | /** |
||
| 346 | * @Route("/adventures", name="adventures") |
||
| 347 | * |
||
| 348 | * @return Response |
||
| 349 | */ |
||
| 350 | public function adventureListAction() |
||
| 378 | |||
| 379 | /** |
||
| 380 | * @Route("/adventures/{id}", name="viewadventure") |
||
| 381 | * |
||
| 382 | * @param int $id |
||
| 383 | * |
||
| 384 | * @return Response |
||
| 385 | */ |
||
| 386 | public function adventureViewAction($id) |
||
| 411 | |||
| 412 | /** |
||
| 413 | * @Route("/leaderboard", name="leaderboard") |
||
| 414 | * |
||
| 415 | * @return Response |
||
| 416 | */ |
||
| 417 | public function leaderboardAction() |
||
| 425 | |||
| 426 | /** |
||
| 427 | * @Route("/search/users", name="search_users") |
||
| 428 | * @param Request $request |
||
| 429 | * |
||
| 430 | * @return JsonResponse |
||
| 431 | */ |
||
| 432 | public function searchUsersAction(Request $request) |
||
| 454 | } |
||
| 455 |
Loading history...
This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.
Both the
$myVarassignment in line 1 and the$higherassignment in line 2 are dead. The first because$myVaris never used and the second because$higheris always overwritten for every possible time line.