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 ParallelExecutor 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 ParallelExecutor, and based on these observations, apply Extract Interface, too.
| 1 | <?php declare(strict_types=1); |
||
| 31 | class ParallelExecutor |
||
| 32 | { |
||
| 33 | private $input; |
||
| 34 | private $output; |
||
| 35 | private $messenger; |
||
| 36 | private $client; |
||
| 37 | private $config; |
||
| 38 | |||
| 39 | 7 | public function __construct( |
|
| 53 | |||
| 54 | /** |
||
| 55 | * @param Host[] $hosts |
||
| 56 | */ |
||
| 57 | 7 | private function connect(array $hosts) |
|
| 84 | |||
| 85 | /** |
||
| 86 | * @param Task[] $tasks |
||
| 87 | * @param Host[] $hosts |
||
| 88 | */ |
||
| 89 | 7 | public function run(array $tasks, array $hosts, $plan = null): int |
|
| 90 | { |
||
| 91 | 7 | $plan || $this->connect($hosts); |
|
| 92 | |||
| 93 | 7 | $localhost = new Localhost(); |
|
|
|
|||
| 94 | 7 | $globalLimit = (int)$this->input->getOption('limit') ?: count($hosts); |
|
| 95 | |||
| 96 | 7 | foreach ($tasks as $task) { |
|
| 97 | 7 | $plan || $this->messenger->startTask($task); |
|
| 98 | |||
| 99 | 7 | $limit = min($globalLimit, $task->getLimit() ?? $globalLimit); |
|
| 100 | 7 | $selector = $task->getSelector() ?? Selector::parse('all'); |
|
| 101 | |||
| 102 | 7 | if ($limit === 1 || count($hosts) === 1) { |
|
| 103 | 6 | foreach ($hosts as $host) { |
|
| 104 | 6 | if (!Selector::apply($selector, $host)) { |
|
| 105 | if ($plan) { |
||
| 106 | $plan->commit([], $task); |
||
| 107 | } |
||
| 108 | continue; |
||
| 109 | } |
||
| 110 | |||
| 111 | 6 | if ($plan) { |
|
| 112 | $plan->commit([$host], $task); |
||
| 113 | continue; |
||
| 114 | } |
||
| 115 | |||
| 116 | try { |
||
| 117 | 6 | $host->getConfig()->load(); |
|
| 118 | 6 | Exception::setTaskSourceLocation($task->getSourceLocation()); |
|
| 119 | |||
| 120 | 6 | $task->run(new Context($host, $this->input, $this->output)); |
|
| 121 | |||
| 122 | 6 | $host->getConfig()->save(); |
|
| 123 | 1 | } catch (GracefulShutdownException $exception) { |
|
| 124 | $this->messenger->renderException($exception, $host); |
||
| 125 | return GracefulShutdownException::EXIT_CODE; |
||
| 126 | 1 | } catch (\Throwable $exception) { |
|
| 127 | 1 | $this->messenger->renderException($exception, $host); |
|
| 128 | 1 | return 1; |
|
| 129 | } |
||
| 130 | } |
||
| 131 | } else { |
||
| 132 | 1 | foreach (array_chunk($hosts, $limit) as $chunk) { |
|
| 133 | 1 | $exitCode = $this->runTask($chunk, $task, $plan); |
|
| 134 | 1 | if ($exitCode !== 0) { |
|
| 135 | return $exitCode; |
||
| 136 | } |
||
| 137 | } |
||
| 138 | } |
||
| 139 | |||
| 140 | 7 | $this->messenger->endTask($task); |
|
| 141 | } |
||
| 142 | |||
| 143 | 7 | return 0; |
|
| 144 | } |
||
| 145 | |||
| 146 | 1 | private function runTask(array $hosts, Task $task, Planner $plan = null): int |
|
| 185 | |||
| 186 | 1 | protected function getProcess(Host $host, Task $task): Process |
|
| 199 | |||
| 200 | /** |
||
| 201 | * @param Process[] $processes |
||
| 202 | */ |
||
| 203 | 1 | protected function startProcesses(array $processes) |
|
| 209 | |||
| 210 | /** |
||
| 211 | * @param Process[] $processes |
||
| 212 | */ |
||
| 213 | 1 | protected function areRunning(array $processes): bool |
|
| 223 | |||
| 224 | /** |
||
| 225 | * @param Process[] $processes |
||
| 226 | */ |
||
| 227 | 1 | protected function gatherOutput(array $processes, callable $callback) |
|
| 241 | |||
| 242 | /** |
||
| 243 | * Gather the cumulative exit code for the processes. |
||
| 244 | */ |
||
| 245 | 1 | protected function cumulativeExitCode(array $processes): int |
|
| 255 | } |
||
| 256 |
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.