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 FilesystemRepository 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 FilesystemRepository, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
64 | class FilesystemRepository extends AbstractEditableRepository |
||
65 | { |
||
66 | /** |
||
67 | * @var bool|null |
||
68 | */ |
||
69 | private static $symlinkSupported; |
||
70 | |||
71 | /** |
||
72 | * @var string |
||
73 | */ |
||
74 | private $baseDir; |
||
75 | |||
76 | /** |
||
77 | * @var bool |
||
78 | */ |
||
79 | private $symlink; |
||
80 | |||
81 | /** |
||
82 | * @var bool |
||
83 | */ |
||
84 | private $relative; |
||
85 | |||
86 | /** |
||
87 | * @var Filesystem |
||
88 | */ |
||
89 | private $filesystem; |
||
90 | |||
91 | /** |
||
92 | * Returns whether symlinks are supported in the local environment. |
||
93 | * |
||
94 | * @return bool Returns `true` if symlinks are supported. |
||
95 | */ |
||
96 | 267 | public static function isSymlinkSupported() |
|
111 | |||
112 | /** |
||
113 | * Creates a new repository. |
||
114 | * |
||
115 | * @param string $baseDir The base directory of the repository on the file |
||
116 | * system. |
||
117 | * @param bool $symlink Whether to use symbolic links for added files. If |
||
118 | * symbolic links are not supported on the current |
||
119 | * system, the repository will create hard copies |
||
120 | * instead. |
||
121 | * @param bool $relative Whether to create relative symbolic links. If |
||
122 | * relative links are not supported on the current |
||
123 | * system, the repository will create absolute links |
||
124 | * instead. |
||
125 | * @param ChangeStream|null $changeStream If provided, the repository will log |
||
126 | * resources changes in this change stream. |
||
127 | */ |
||
128 | 344 | public function __construct($baseDir = '/', $symlink = true, $relative = true, ChangeStream $changeStream = null) |
|
129 | { |
||
130 | 344 | parent::__construct($changeStream); |
|
131 | |||
132 | 344 | Assert::directory($baseDir); |
|
133 | 344 | Assert::boolean($symlink); |
|
134 | |||
135 | 344 | $this->baseDir = rtrim(Path::canonicalize($baseDir), '/'); |
|
136 | 344 | $this->symlink = $symlink && self::isSymlinkSupported(); |
|
137 | 344 | $this->relative = $this->symlink && $relative; |
|
138 | 344 | $this->filesystem = new Filesystem(); |
|
139 | 344 | } |
|
140 | |||
141 | /** |
||
142 | * {@inheritdoc} |
||
143 | */ |
||
144 | 135 | View Code Duplication | public function get($path) |
155 | |||
156 | /** |
||
157 | * {@inheritdoc} |
||
158 | */ |
||
159 | 41 | public function find($query, $language = 'glob') |
|
163 | |||
164 | /** |
||
165 | * {@inheritdoc} |
||
166 | */ |
||
167 | 61 | public function contains($query, $language = 'glob') |
|
174 | |||
175 | /** |
||
176 | * {@inheritdoc} |
||
177 | */ |
||
178 | 28 | public function hasChildren($path) |
|
191 | |||
192 | /** |
||
193 | * {@inheritdoc} |
||
194 | */ |
||
195 | 42 | public function listChildren($path) |
|
205 | |||
206 | /** |
||
207 | * {@inheritdoc} |
||
208 | */ |
||
209 | 305 | public function add($path, $resource) |
|
234 | |||
235 | /** |
||
236 | * {@inheritdoc} |
||
237 | */ |
||
238 | 49 | public function remove($query, $language = 'glob') |
|
252 | |||
253 | /** |
||
254 | * {@inheritdoc} |
||
255 | */ |
||
256 | 4 | public function clear() |
|
267 | |||
268 | 289 | private function ensureDirectoryExists($path) |
|
284 | |||
285 | 289 | private function addResource($path, PuliResource $resource, $checkParentsForSymlinks = true) |
|
286 | { |
||
287 | 289 | $pathInBaseDir = $this->baseDir.$path; |
|
288 | 289 | $hasChildren = $resource->hasChildren(); |
|
289 | 289 | $hasBody = $resource instanceof BodyResource; |
|
290 | |||
291 | 289 | if ($hasChildren && $hasBody) { |
|
292 | 1 | throw new UnsupportedResourceException(sprintf( |
|
293 | 'Instances of BodyResource do not support child resources in '. |
||
294 | 1 | 'FilesystemRepository. Tried to add a BodyResource with '. |
|
295 | 1 | 'children at %s.', |
|
296 | $path |
||
297 | 1 | )); |
|
298 | } |
||
299 | |||
300 | // Don't modify resources attached to other repositories |
||
301 | 288 | if ($resource->isAttached()) { |
|
302 | 4 | $resource = clone $resource; |
|
303 | 4 | } |
|
304 | |||
305 | 288 | $resource->attachTo($this, $path); |
|
306 | |||
307 | 288 | if ($this->symlink && $checkParentsForSymlinks) { |
|
308 | 156 | $this->replaceParentSymlinksByCopies($path); |
|
309 | 156 | } |
|
310 | |||
311 | 288 | if ($resource instanceof FilesystemResource) { |
|
312 | 37 | if ($this->symlink) { |
|
313 | 32 | $this->symlinkMirror($resource->getFilesystemPath(), $pathInBaseDir); |
|
314 | 37 | } elseif ($hasBody) { |
|
315 | 4 | $this->filesystem->copy($resource->getFilesystemPath(), $pathInBaseDir); |
|
316 | 4 | } else { |
|
317 | 1 | $this->filesystem->mirror($resource->getFilesystemPath(), $pathInBaseDir); |
|
318 | } |
||
319 | |||
320 | 37 | $this->appendToChangeStream($resource); |
|
321 | |||
322 | 37 | return; |
|
323 | } |
||
324 | |||
325 | 259 | if ($resource instanceof LinkResource) { |
|
326 | 8 | if (!$this->symlink) { |
|
327 | 4 | throw new UnsupportedResourceException(sprintf( |
|
328 | 'LinkResource requires support of symbolic links in FilesystemRepository. '. |
||
329 | 4 | 'Tried to add a LinkResource at %s.', |
|
330 | $path |
||
331 | 4 | )); |
|
332 | } |
||
333 | |||
334 | 4 | $this->filesystem->symlink($this->baseDir.$resource->getTargetPath(), $pathInBaseDir); |
|
335 | |||
336 | 4 | $this->appendToChangeStream($resource); |
|
337 | |||
338 | 4 | return; |
|
339 | } |
||
340 | |||
341 | 251 | if ($hasBody) { |
|
342 | 127 | file_put_contents($pathInBaseDir, $resource->getBody()); |
|
|
|||
343 | |||
344 | 127 | $this->appendToChangeStream($resource); |
|
345 | |||
346 | 127 | return; |
|
347 | } |
||
348 | |||
349 | 191 | if (is_file($pathInBaseDir)) { |
|
350 | $this->filesystem->remove($pathInBaseDir); |
||
351 | } |
||
352 | |||
353 | 191 | if (!file_exists($pathInBaseDir)) { |
|
354 | 115 | mkdir($pathInBaseDir, 0777, true); |
|
355 | 115 | } |
|
356 | |||
357 | 191 | foreach ($resource->listChildren() as $child) { |
|
358 | 112 | $this->addResource($path.'/'.$child->getName(), $child, false); |
|
359 | 191 | } |
|
360 | |||
361 | 191 | $this->appendToChangeStream($resource); |
|
362 | 191 | } |
|
363 | |||
364 | 32 | private function removeResource($filesystemPath, &$removed) |
|
379 | |||
380 | 119 | private function createResource($filesystemPath, $path) |
|
406 | |||
407 | 25 | private function countChildren($filesystemPath) |
|
424 | |||
425 | 50 | private function iteratorToCollection(Iterator $iterator) |
|
448 | |||
449 | 66 | View Code Duplication | private function getFilesystemPath($path) |
460 | |||
461 | 123 | private function getGlobIterator($query, $language) |
|
472 | |||
473 | 70 | private function getDirectoryIterator($filesystemPath) |
|
480 | |||
481 | 32 | private function symlinkMirror($origin, $target, array $dirsToKeep = array()) |
|
525 | |||
526 | 156 | private function replaceParentSymlinksByCopies($path) |
|
561 | |||
562 | 16 | private function replaceLinkByCopy($path, array $dirsToKeep = array()) |
|
569 | |||
570 | 32 | private function trySymlink($origin, $target) |
|
583 | |||
584 | 22 | private function readLink($filesystemPath) |
|
602 | } |
||
603 |
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 implementation 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 interface: