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 PackageManagerImpl 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 PackageManagerImpl, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
41 | class PackageManagerImpl implements PackageManager |
||
42 | { |
||
43 | /** |
||
44 | * @var ProjectContext |
||
45 | */ |
||
46 | private $context; |
||
47 | |||
48 | /** |
||
49 | * @var string |
||
50 | */ |
||
51 | private $rootDir; |
||
52 | |||
53 | /** |
||
54 | * @var RootPackageFile |
||
55 | */ |
||
56 | private $rootPackageFile; |
||
57 | |||
58 | /** |
||
59 | * @var PackageFileStorage |
||
60 | */ |
||
61 | private $packageFileStorage; |
||
62 | |||
63 | /** |
||
64 | * @var PackageCollection |
||
65 | */ |
||
66 | private $packages; |
||
67 | |||
68 | /** |
||
69 | * Loads the package repository for a given project. |
||
70 | * |
||
71 | * @param ProjectContext $context The project context. |
||
72 | * @param PackageFileStorage $packageFileStorage The package file storage. |
||
73 | * |
||
74 | * @throws FileNotFoundException If the install path of a package not exist. |
||
75 | * @throws NoDirectoryException If the install path of a package points to a file. |
||
76 | * @throws InvalidConfigException If a configuration file contains invalid configuration. |
||
77 | * @throws NameConflictException If a package has the same name as another loaded package. |
||
78 | */ |
||
79 | 53 | public function __construct(ProjectContext $context, PackageFileStorage $packageFileStorage) |
|
86 | |||
87 | /** |
||
88 | * {@inheritdoc} |
||
89 | */ |
||
90 | 12 | public function installPackage($installPath, $name = null, $installerName = InstallInfo::DEFAULT_INSTALLER_NAME, $env = Environment::PROD) |
|
91 | { |
||
92 | 12 | Assert::string($installPath, 'The install path must be a string. Got: %s'); |
|
93 | 12 | Assert::string($installerName, 'The installer name must be a string. Got: %s'); |
|
94 | 12 | Assert::oneOf($env, Environment::all(), 'The environment must be one of: %2$s. Got: %s'); |
|
95 | 12 | Assert::nullOrPackageName($name); |
|
96 | |||
97 | 11 | $this->assertPackagesLoaded(); |
|
98 | |||
99 | 11 | $installPath = Path::makeAbsolute($installPath, $this->rootDir); |
|
100 | |||
101 | 11 | foreach ($this->packages as $package) { |
|
102 | 11 | if ($installPath === $package->getInstallPath()) { |
|
103 | 1 | return; |
|
104 | } |
||
105 | 11 | } |
|
106 | |||
107 | 10 | if (null === $name && $packageFile = $this->loadPackageFile($installPath)) { |
|
108 | // Read the name from the package file |
||
109 | 6 | $name = $packageFile->getPackageName(); |
|
110 | 6 | } |
|
111 | |||
112 | 8 | if (null === $name) { |
|
113 | 1 | throw new InvalidConfigException(sprintf( |
|
114 | 'Could not find a name for the package at %s. The name should '. |
||
115 | 1 | 'either be passed to the installer or be set in the "name" '. |
|
116 | 1 | 'property of %s.', |
|
117 | 1 | $installPath, |
|
118 | $installPath.'/puli.json' |
||
119 | 1 | )); |
|
120 | } |
||
121 | |||
122 | 7 | if ($this->packages->contains($name)) { |
|
123 | 1 | throw NameConflictException::forName($name); |
|
124 | } |
||
125 | |||
126 | 6 | $relInstallPath = Path::makeRelative($installPath, $this->rootDir); |
|
127 | 6 | $installInfo = new InstallInfo($name, $relInstallPath); |
|
128 | 6 | $installInfo->setInstallerName($installerName); |
|
129 | 6 | $installInfo->setEnvironment($env); |
|
130 | |||
131 | 6 | $package = $this->loadPackage($installInfo); |
|
132 | |||
133 | 6 | $this->assertNoLoadErrors($package); |
|
134 | 5 | $this->rootPackageFile->addInstallInfo($installInfo); |
|
135 | |||
136 | try { |
||
137 | 5 | $this->packageFileStorage->saveRootPackageFile($this->rootPackageFile); |
|
138 | 5 | } catch (Exception $e) { |
|
139 | $this->rootPackageFile->removeInstallInfo($name); |
||
140 | |||
141 | throw $e; |
||
142 | } |
||
143 | |||
144 | 5 | $this->packages->add($package); |
|
145 | 5 | } |
|
146 | |||
147 | /** |
||
148 | * {@inheritdoc} |
||
149 | */ |
||
150 | 8 | public function renamePackage($name, $newName) |
|
168 | |||
169 | /** |
||
170 | * {@inheritdoc} |
||
171 | */ |
||
172 | 4 | public function removePackage($name) |
|
195 | |||
196 | /** |
||
197 | * {@inheritdoc} |
||
198 | */ |
||
199 | 3 | public function removePackages(Expression $expr) |
|
226 | |||
227 | /** |
||
228 | * {@inheritdoc} |
||
229 | */ |
||
230 | 1 | public function clearPackages() |
|
234 | |||
235 | /** |
||
236 | * {@inheritdoc} |
||
237 | */ |
||
238 | 10 | public function getPackage($name) |
|
246 | |||
247 | /** |
||
248 | * {@inheritdoc} |
||
249 | */ |
||
250 | 1 | public function getRootPackage() |
|
256 | |||
257 | /** |
||
258 | * {@inheritdoc} |
||
259 | */ |
||
260 | 24 | public function getPackages() |
|
267 | |||
268 | /** |
||
269 | * {@inheritdoc} |
||
270 | */ |
||
271 | 4 | public function findPackages(Expression $expr) |
|
285 | |||
286 | /** |
||
287 | * {@inheritdoc} |
||
288 | */ |
||
289 | 11 | public function hasPackage($name) |
|
297 | |||
298 | /** |
||
299 | * {@inheritdoc} |
||
300 | */ |
||
301 | 1 | View Code Duplication | public function hasPackages(Expression $expr = null) |
317 | |||
318 | /** |
||
319 | * {@inheritdoc} |
||
320 | */ |
||
321 | 1 | public function getContext() |
|
325 | |||
326 | /** |
||
327 | * Loads all packages referenced by the install file. |
||
328 | * |
||
329 | * @throws FileNotFoundException If the install path of a package not exist. |
||
330 | * @throws NoDirectoryException If the install path of a package points to a |
||
331 | * file. |
||
332 | * @throws InvalidConfigException If a package is not configured correctly. |
||
333 | * @throws NameConflictException If a package has the same name as another |
||
334 | * loaded package. |
||
335 | */ |
||
336 | 52 | private function loadPackages() |
|
347 | |||
348 | /** |
||
349 | * Loads a package for the given install info. |
||
350 | * |
||
351 | * @param InstallInfo $installInfo The install info. |
||
352 | * |
||
353 | * @return Package The package. |
||
354 | */ |
||
355 | 52 | private function loadPackage(InstallInfo $installInfo) |
|
373 | |||
374 | /** |
||
375 | * Loads the package file for the package at the given install path. |
||
376 | * |
||
377 | * @param string $installPath The absolute install path of the package |
||
378 | * |
||
379 | * @return PackageFile|null The loaded package file or `null` if none |
||
380 | * could be found. |
||
381 | */ |
||
382 | 52 | private function loadPackageFile($installPath) |
|
402 | |||
403 | 52 | private function assertPackagesLoaded() |
|
409 | |||
410 | 6 | private function assertNoLoadErrors(Package $package) |
|
419 | |||
420 | 2 | private function renameRootPackage(RootPackage $package, $newName) |
|
437 | |||
438 | 2 | private function renameNonRootPackage(Package $package, $newName) |
|
469 | } |
||
470 |
This check marks implicit conversions of arrays to boolean values in a comparison. While in PHP an empty array is considered to be equal (but not identical) to false, this is not always apparent.
Consider making the comparison explicit by using
empty(..)
or! empty(...)
instead.