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 Build 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 Build, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
19 | class Build extends BaseBuild |
||
20 | { |
||
21 | const STAGE_SETUP = 'setup'; |
||
22 | const STAGE_TEST = 'test'; |
||
23 | const STAGE_DEPLOY = 'deploy'; |
||
24 | const STAGE_COMPLETE = 'complete'; |
||
25 | const STAGE_SUCCESS = 'success'; |
||
26 | const STAGE_FAILURE = 'failure'; |
||
27 | const STAGE_FIXED = 'fixed'; |
||
28 | const STAGE_BROKEN = 'broken'; |
||
29 | |||
30 | /** |
||
31 | * @var array |
||
32 | */ |
||
33 | public static $pullRequestSources = [ |
||
34 | self::SOURCE_WEBHOOK_PULL_REQUEST_CREATED, |
||
35 | self::SOURCE_WEBHOOK_PULL_REQUEST_UPDATED, |
||
36 | self::SOURCE_WEBHOOK_PULL_REQUEST_APPROVED, |
||
37 | self::SOURCE_WEBHOOK_PULL_REQUEST_MERGED, |
||
38 | ]; |
||
39 | |||
40 | /** |
||
41 | * @var array |
||
42 | */ |
||
43 | public static $webhookSources = [ |
||
44 | self::SOURCE_WEBHOOK_PUSH, |
||
45 | self::SOURCE_WEBHOOK_PULL_REQUEST_CREATED, |
||
46 | self::SOURCE_WEBHOOK_PULL_REQUEST_UPDATED, |
||
47 | self::SOURCE_WEBHOOK_PULL_REQUEST_APPROVED, |
||
48 | self::SOURCE_WEBHOOK_PULL_REQUEST_MERGED, |
||
49 | ]; |
||
50 | |||
51 | /** |
||
52 | * @var array |
||
53 | */ |
||
54 | protected $totalErrorsCount = []; |
||
55 | |||
56 | /** |
||
57 | * @var string |
||
58 | */ |
||
59 | protected $buildDirectory; |
||
60 | |||
61 | /** |
||
62 | * @var string |
||
63 | */ |
||
64 | protected $buildBranchDirectory; |
||
65 | |||
66 | /** |
||
67 | * @return null|Project |
||
68 | * |
||
69 | * @throws \PHPCensor\Exception\HttpException |
||
70 | */ |
||
71 | public function getProject() |
||
85 | |||
86 | /** |
||
87 | * @param string $name |
||
88 | * @param mixed $value |
||
89 | */ |
||
90 | public function addExtraValue($name, $value) |
||
100 | |||
101 | /** |
||
102 | * @param string $name |
||
103 | */ |
||
104 | public function removeExtraValue($name) |
||
113 | |||
114 | /** |
||
115 | * Get BuildError models by BuildId for this Build. |
||
116 | * |
||
117 | * @return \PHPCensor\Model\BuildError[] |
||
118 | */ |
||
119 | public function getBuildBuildErrors() |
||
123 | |||
124 | /** |
||
125 | * Get BuildMeta models by BuildId for this Build. |
||
126 | * |
||
127 | * @return \PHPCensor\Model\BuildMeta[] |
||
128 | */ |
||
129 | public function getBuildBuildMetas() |
||
133 | |||
134 | /** |
||
135 | * Get link to commit from another source (i.e. Github) |
||
136 | */ |
||
137 | public function getCommitLink() |
||
141 | |||
142 | /** |
||
143 | * Get link to branch from another source (i.e. Github) |
||
144 | */ |
||
145 | public function getBranchLink() |
||
149 | |||
150 | /** |
||
151 | * Get remote branch (from pull request) from another source (i.e. Github) |
||
152 | */ |
||
153 | public function getRemoteBranch() |
||
157 | |||
158 | /** |
||
159 | * Get link to remote branch (from pull request) from another source (i.e. Github) |
||
160 | */ |
||
161 | public function getRemoteBranchLink() |
||
165 | |||
166 | /** |
||
167 | * Get link to tag from another source (i.e. Github) |
||
168 | */ |
||
169 | public function getTagLink() |
||
173 | |||
174 | /** |
||
175 | * Return a template to use to generate a link to a specific file. |
||
176 | * |
||
177 | * @return string|null |
||
178 | */ |
||
179 | public function getFileLinkTemplate() |
||
183 | |||
184 | /** |
||
185 | * Send status updates to any relevant third parties (i.e. Github) |
||
186 | */ |
||
187 | public function sendStatusPostback() |
||
191 | |||
192 | /** |
||
193 | * @return string |
||
194 | * |
||
195 | * @throws \PHPCensor\Exception\HttpException |
||
196 | */ |
||
197 | public function getProjectTitle() |
||
202 | |||
203 | /** |
||
204 | * Store build metadata |
||
205 | * |
||
206 | * @param string $key |
||
207 | * @param mixed $value |
||
208 | */ |
||
209 | public function storeMeta($key, $value) |
||
215 | |||
216 | /** |
||
217 | * Is this build successful? |
||
218 | */ |
||
219 | public function isSuccessful() |
||
223 | |||
224 | /** |
||
225 | * @param Builder $builder |
||
226 | * |
||
227 | * @return bool |
||
228 | * |
||
229 | * @throws \Exception |
||
230 | */ |
||
231 | public function handleConfigBeforeClone(Builder $builder) |
||
248 | |||
249 | /** |
||
250 | * @param Builder $builder |
||
251 | * @param string $buildPath |
||
252 | * |
||
253 | * @return bool |
||
254 | * |
||
255 | * @throws \Exception |
||
256 | */ |
||
257 | protected function handleConfig(Builder $builder, $buildPath) |
||
321 | |||
322 | /** |
||
323 | * Get an array of plugins to run if there's no .php-censor.yml file. |
||
324 | * |
||
325 | * @param Builder $builder |
||
326 | * |
||
327 | * @return array |
||
328 | * |
||
329 | * @throws \ReflectionException |
||
330 | */ |
||
331 | protected function getZeroConfigPlugins(Builder $builder) |
||
382 | |||
383 | /** |
||
384 | * Allows specific build types (e.g. Github) to report violations back to their respective services. |
||
385 | * |
||
386 | * @param Builder $builder |
||
387 | * @param string $plugin |
||
388 | * @param string $message |
||
389 | * @param int $severity |
||
390 | * @param string $file |
||
391 | * @param int $lineStart |
||
392 | * @param int $lineEnd |
||
393 | */ |
||
394 | public function reportError( |
||
413 | |||
414 | /** |
||
415 | * @return string|null |
||
416 | */ |
||
417 | View Code Duplication | public function getBuildDirectory() |
|
436 | |||
437 | /** |
||
438 | * @return string|null |
||
439 | */ |
||
440 | View Code Duplication | public function getBuildBranchDirectory() |
|
459 | |||
460 | /** |
||
461 | * @return string|null |
||
462 | */ |
||
463 | public function getBuildPath() |
||
474 | |||
475 | /** |
||
476 | * Removes the build directory. |
||
477 | * |
||
478 | * @param bool $withArtifacts |
||
479 | */ |
||
480 | public function removeBuildDirectory($withArtifacts = false) |
||
509 | |||
510 | /** |
||
511 | * Get the number of seconds a build has been running for. |
||
512 | * |
||
513 | * @return int |
||
514 | */ |
||
515 | public function getDuration() |
||
531 | |||
532 | /** |
||
533 | * get time a build has been running for in hour/minute/seconds format (e.g. 1h 21m 45s) |
||
534 | * |
||
535 | * @return string |
||
536 | */ |
||
537 | public function getPrettyDuration() |
||
558 | |||
559 | /** |
||
560 | * Create a working copy by cloning, copying, or similar. |
||
561 | * |
||
562 | * @param Builder $builder |
||
563 | * @param string $buildPath |
||
564 | * |
||
565 | * @return bool |
||
566 | */ |
||
567 | public function createWorkingCopy(Builder $builder, $buildPath) |
||
571 | |||
572 | /** |
||
573 | * Create an SSH key file on disk for this build. |
||
574 | * |
||
575 | * @return string |
||
576 | * |
||
577 | * @throws \PHPCensor\Exception\HttpException |
||
578 | */ |
||
579 | protected function writeSshKey() |
||
587 | |||
588 | /** |
||
589 | * Create an SSH wrapper script for Svn to use, to disable host key checking, etc. |
||
590 | * |
||
591 | * @param string $keyFile |
||
592 | * |
||
593 | * @return string |
||
594 | */ |
||
595 | protected function writeSshWrapper($keyFile) |
||
612 | |||
613 | /** |
||
614 | * @return string |
||
615 | */ |
||
616 | public function getSourceHumanize() |
||
647 | |||
648 | /** |
||
649 | * Gets the total number of errors for a given build. |
||
650 | * |
||
651 | * @param string|null $plugin |
||
652 | * @param int|null $severity |
||
653 | * @param string|null $isNew |
||
654 | * |
||
655 | * @return int |
||
656 | * |
||
657 | * @throws \Exception |
||
658 | */ |
||
659 | public function getTotalErrorsCount($plugin = null, $severity = null, $isNew = null) |
||
686 | |||
687 | /** |
||
688 | * @return int |
||
689 | * |
||
690 | * @throws InvalidArgumentException |
||
691 | * @throws \PHPCensor\Exception\HttpException |
||
692 | */ |
||
693 | public function getErrorsTrend() |
||
711 | } |
||
712 |
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: