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 MatchController 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 MatchController, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
8 | class MatchController extends CRUDController |
||
9 | { |
||
10 | /** |
||
11 | * Whether the last edited match has had its ELO changed, requiring an ELO |
||
12 | * recalculation |
||
13 | * |
||
14 | * This is useful so that a confirmation form is shown, asking the user if |
||
15 | * they want to recalculate ELOs |
||
16 | * |
||
17 | * @var bool |
||
18 | */ |
||
19 | public $recalculateNeeded = false; |
||
20 | |||
21 | public function listAction(Request $request, Player $me, Team $team = null, Player $player = null, $type = null) |
||
22 | { |
||
23 | $qb = $this->getQueryBuilder(); |
||
24 | |||
25 | $currentPage = $request->query->get('page', 1); |
||
26 | |||
27 | if ($player) { |
||
28 | $team = $player; |
||
29 | } |
||
30 | |||
31 | $query = $qb->sortBy('time')->reverse() |
||
|
|||
32 | ->with($team, $type) |
||
33 | ->limit(50)->fromPage($currentPage); |
||
34 | |||
35 | $matchType = $request->query->get('type', 'all'); |
||
36 | |||
37 | if (in_array($matchType, array(Match::FUN, Match::OFFICIAL, Match::SPECIAL))) { |
||
38 | $query->where('type')->is($matchType); |
||
39 | } |
||
40 | |||
41 | $matches = $query->getModels($fast = true); |
||
42 | |||
43 | foreach ($matches as $match) { |
||
44 | // Don't show wrong labels for matches |
||
45 | $match->getOriginalTimestamp()->setTimezone($me->getTimezone()); |
||
46 | } |
||
47 | |||
48 | return array( |
||
49 | "matches" => $matches, |
||
50 | "team" => $team, |
||
51 | "currentPage" => $currentPage, |
||
52 | "totalPages" => $qb->countPages() |
||
53 | ); |
||
54 | } |
||
55 | |||
56 | 1 | public function showAction(Match $match) |
|
60 | |||
61 | 1 | View Code Duplication | public function createAction(Player $me) |
62 | { |
||
63 | return $this->create($me, function (Match $match) use ($me) { |
||
64 | 1 | if ($me->canEdit($match) |
|
65 | 1 | && $match->isOfficial() |
|
66 | 1 | && (!$match->getTeamA()->isLastMatch($match) |
|
67 | 1 | || !$match->getTeamB()->isLastMatch($match)) |
|
68 | ) { |
||
69 | $url = Service::getGenerator()->generate('match_recalculate', array( |
||
70 | 'match' => $match->getId(), |
||
71 | )); |
||
72 | |||
73 | return new RedirectResponse($url); |
||
74 | } |
||
75 | 1 | }); |
|
76 | } |
||
77 | |||
78 | View Code Duplication | public function deleteAction(Player $me, Match $match) |
|
79 | { |
||
80 | return $this->delete($match, $me, function () use ($match, $me) { |
||
81 | if ($match->getTeamA()->isLastMatch($match) |
||
82 | && $match->getTeamB()->isLastMatch($match)) { |
||
83 | $match->resetELOs(); |
||
84 | } elseif ($me->canEdit($match)) { |
||
85 | $url = Service::getGenerator()->generate('match_recalculate', array( |
||
86 | 'match' => $match->getId(), |
||
87 | )); |
||
88 | |||
89 | return new RedirectResponse($url); |
||
90 | } |
||
91 | }); |
||
92 | } |
||
93 | |||
94 | View Code Duplication | public function editAction(Player $me, Match $match) |
|
95 | { |
||
96 | // TODO: Generating this response is unnecessary |
||
97 | $response = $this->edit($match, $me, "match"); |
||
98 | |||
99 | if ($this->recalculateNeeded && $match->isOfficial()) { |
||
100 | // Redirect to a confirmation form if we are assigning a new leader |
||
101 | $url = Service::getGenerator()->generate('match_recalculate', array( |
||
102 | 'match' => $match->getId(), |
||
103 | )); |
||
104 | |||
105 | return new RedirectResponse($url); |
||
106 | } |
||
107 | |||
108 | return $response; |
||
109 | } |
||
110 | |||
111 | public function recalculateAction(Player $me, $match) |
||
144 | |||
145 | /** |
||
146 | * Recalculates match history for all teams and matches |
||
147 | * |
||
148 | * Recalculation is done as follows: |
||
149 | * 1. A match is chosen as a starting point - it's stored old team ELOs are |
||
150 | * considered correct |
||
151 | * 2. Team ELOs are reset to their values at the starting point |
||
152 | * 3. Each match that occurred since the first specified match has its ELO |
||
153 | * recalculated based on the current team values, and the new match data |
||
154 | * and team ELOs are stored in the database |
||
155 | * |
||
156 | * @param Match $match The first match |
||
157 | */ |
||
158 | private function recalculate(Match $match) |
||
159 | { |
||
160 | try { |
||
161 | // Commented out to prevent ridiculously large recalculations |
||
162 | //set_time_limit(0); |
||
163 | |||
164 | $query = Match::getQueryBuilder() |
||
165 | ->where('status')->notEquals('deleted') |
||
166 | ->where('type')->equals(Match::OFFICIAL) |
||
167 | ->where('time')->isAfter($match->getTimestamp(), $inclusive = true) |
||
168 | ->sortBy('time'); |
||
169 | |||
170 | /** @var Match[] $matches */ |
||
171 | $matches = $query->getModels($fast = true); |
||
172 | |||
173 | // Send the total count to client-side javascript |
||
174 | $this->log(count($matches) . "\n"); |
||
175 | |||
176 | // Start a transaction so tables are locked and we don't stay with |
||
177 | // messed up data if something goes wrong |
||
178 | Database::getInstance()->startTransaction(); |
||
179 | |||
180 | $teamsReset = []; |
||
181 | |||
182 | // Reset match teams, in case the selected match is deleted and does |
||
183 | // not show up in the list of matches to recalculate |
||
184 | $match->getTeamA()->setElo($match->getTeamAEloOld()); |
||
185 | $match->getTeamB()->setElo($match->getTeamBEloOld()); |
||
186 | $teamsReset[ $match->getTeamA()->getId() ] = true; |
||
187 | $teamsReset[ $match->getTeamB()->getId() ] = true; |
||
188 | |||
189 | foreach ($matches as $i => $match) { |
||
190 | // Reset teams' ELOs if they haven't been reset already |
||
191 | View Code Duplication | if (!isset($teamsReset[ $match->getTeamA()->getId() ])) { |
|
192 | $teamsReset[ $match->getTeamA()->getId() ] = true; |
||
193 | $match->getTeamA()->setElo($match->getTeamAEloOld()); |
||
194 | } |
||
195 | View Code Duplication | if (!isset($teamsReset[ $match->getTeamB()->getId() ])) { |
|
196 | $teamsReset[ $match->getTeamB()->getId() ] = true; |
||
197 | $match->getTeamB()->setElo($match->getTeamBEloOld()); |
||
198 | } |
||
199 | |||
200 | $match->recalculateElo(); |
||
201 | |||
202 | // Send an update to the client-side javascript, so that a |
||
203 | // progress bar can be updated |
||
204 | $this->log("m"); |
||
205 | } |
||
206 | } catch (Exception $e) { |
||
207 | Database::getInstance()->rollback(); |
||
208 | Database::getInstance()->finishTransaction(); |
||
209 | throw $e; |
||
210 | } |
||
211 | |||
212 | Database::getInstance()->finishTransaction(); |
||
213 | |||
214 | $this->log("\n\nCalculation successful\n"); |
||
215 | } |
||
216 | |||
217 | /** |
||
218 | * Echo a string and flush the buffers |
||
219 | * |
||
220 | * Useful for streamed AJAX responses |
||
221 | * |
||
222 | * @param string $string The string to echo |
||
223 | */ |
||
224 | private function log($string) |
||
230 | |||
231 | /** |
||
232 | * {@inheritdoc} |
||
233 | */ |
||
234 | 1 | protected function getMessages($type, $name = '') |
|
249 | |||
250 | 1 | protected function validate($form) |
|
290 | |||
291 | protected function validateEdit($form, $match) |
||
301 | } |
||
302 |
Let’s take a look at an example:
In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.
Available Fixes
Change the type-hint for the parameter:
Add an additional type-check:
Add the method to the parent class: