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 ImportFactory 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 ImportFactory, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
31 | class ImportFactory implements EventSubscriberInterface |
||
32 | { |
||
33 | /** |
||
34 | * @var ManagerRegistry |
||
35 | */ |
||
36 | protected $doctrine; |
||
37 | |||
38 | /** |
||
39 | * @var ImportRegistry |
||
40 | */ |
||
41 | protected $importRegistry; |
||
42 | |||
43 | /** |
||
44 | * @var ImporterBuilderFactory |
||
45 | */ |
||
46 | protected $importerBuilderFactory; |
||
47 | |||
48 | /** |
||
49 | * @var ReaderBuilderFactory |
||
50 | */ |
||
51 | protected $readerBuilderFactory; |
||
52 | |||
53 | /** |
||
54 | * @var ImportStorage |
||
55 | */ |
||
56 | protected $importStorage; |
||
57 | |||
58 | /** |
||
59 | * @var EventDispatcherInterface |
||
60 | */ |
||
61 | protected $eventDispatcher; |
||
62 | |||
63 | /** |
||
64 | * @param ManagerRegistry $doctrine |
||
65 | * @param ImportRegistry $importRegistry |
||
66 | * @param ImporterBuilderFactory $importerBuilderFactory |
||
67 | * @param ReaderBuilderFactory $readerBuilderFactory |
||
68 | * @param ImportStorage $importStorage |
||
69 | * @param EventDispatcherInterface $dispatcher |
||
70 | */ |
||
71 | 4 | public function __construct( |
|
72 | ManagerRegistry $doctrine, |
||
73 | ImportRegistry $importRegistry, |
||
74 | ImporterBuilderFactory $importerBuilderFactory, |
||
75 | ReaderBuilderFactory $readerBuilderFactory, |
||
76 | ImportStorage $importStorage, |
||
77 | EventDispatcherInterface $dispatcher |
||
78 | ) { |
||
79 | 4 | $this->doctrine = $doctrine; |
|
80 | 4 | $this->importRegistry = $importRegistry; |
|
81 | 4 | $this->importerBuilderFactory = $importerBuilderFactory; |
|
82 | 4 | $this->readerBuilderFactory = $readerBuilderFactory; |
|
83 | 4 | $this->importStorage = $importStorage; |
|
84 | 4 | $this->eventDispatcher = $dispatcher; |
|
85 | 4 | } |
|
86 | |||
87 | /** |
||
88 | * @return EventDispatcherInterface |
||
89 | */ |
||
90 | public function getEventDispatcher() |
||
91 | { |
||
92 | return $this->eventDispatcher; |
||
93 | } |
||
94 | |||
95 | /** |
||
96 | * @inheritdoc |
||
97 | */ |
||
98 | 4 | public static function getSubscribedEvents() |
|
99 | { |
||
100 | 4 | $events = []; |
|
101 | |||
102 | 4 | foreach ([FeedEvents::class, ImportEvents::class] as $class) { |
|
103 | 4 | $refl = new \ReflectionClass($class); |
|
104 | 4 | foreach ($refl->getConstants() as $constant) { |
|
105 | 4 | $events[$constant][] = 'relayEvent'; |
|
106 | } |
||
107 | } |
||
108 | |||
109 | 4 | $events[ImportEvents::EXCEPTION][] = 'onException'; |
|
110 | |||
111 | 4 | return $events; |
|
112 | } |
||
113 | |||
114 | /** |
||
115 | * Relays an event to the main dispatcher in the manager. |
||
116 | * This is done so listeners can subscribe to this class, |
||
117 | * while each importer starts with a new dispatcher. |
||
118 | |||
119 | * @param Event $event |
||
120 | * @param string $name |
||
121 | */ |
||
122 | 4 | public function relayEvent(Event $event, $name) |
|
126 | |||
127 | /** |
||
128 | * Handler for an exception event. Importer types can listen to the same |
||
129 | * event and stop propagation if they want to change this behaviour. |
||
130 | * |
||
131 | * @param ExceptionEvent $event |
||
132 | * |
||
133 | * @throws \RuntimeException |
||
134 | */ |
||
135 | public function onException(ExceptionEvent $event) |
||
136 | { |
||
137 | $exception = $event->getException(); |
||
138 | if ($exception instanceof ReadException) { |
||
139 | $msg = sprintf('Error reading feed: %s', $exception->getMessage()); |
||
140 | } else { |
||
141 | $msg = sprintf( |
||
142 | 'Import aborted with %s: "%s" Stack trace: %s', |
||
143 | get_class($exception), |
||
144 | $exception->getMessage(), |
||
145 | $exception->getTraceAsString() |
||
146 | ); |
||
147 | } |
||
148 | |||
149 | throw new \RuntimeException($msg, 0, $exception); |
||
150 | } |
||
151 | |||
152 | /** |
||
153 | * @param ItemLoggerInterface $logger |
||
154 | */ |
||
155 | 4 | public function setItemLogger(ItemLoggerInterface $logger) |
|
159 | |||
160 | /** |
||
161 | * Creates an import for a feed. If an import for this feed was created |
||
162 | * before, but has not started yet, that import is returned. All other open |
||
163 | * imports are closed first. |
||
164 | * |
||
165 | * @param FeedEntity $feed The feed to create the import for |
||
166 | * @param \DateTime $scheduleDate The date this import should start |
||
167 | * @param bool $forced Whether to handle items that would normally be skipped |
||
168 | * @param bool $partial If left out, it will be determined based on feed |
||
169 | * |
||
170 | * @throws UnfinishedImportException When an existing (and running) import is found |
||
171 | * |
||
172 | * @return Import |
||
173 | */ |
||
174 | 4 | public function createImport(FeedEntity $feed, \DateTime $scheduleDate = null, $forced = false, $partial = null) |
|
175 | { |
||
176 | 4 | if (is_null($scheduleDate)) { |
|
177 | $scheduleDate = new \DateTime(); |
||
178 | } |
||
179 | |||
180 | 4 | if (is_null($partial)) { |
|
181 | 4 | $partial = $feed->isPartial(); |
|
182 | } |
||
183 | // see if any imports are still unfinished |
||
184 | 4 | $import = $this->findOrCreateImport($feed); |
|
185 | |||
186 | 4 | $exception = null; |
|
187 | |||
188 | // check if it's a new import |
||
189 | 4 | if (!$import->getId()) { |
|
190 | 4 | $import->setForced($forced); |
|
191 | 4 | $import->setPartial($partial); |
|
192 | 4 | $import->setDatetimeScheduled($scheduleDate); |
|
193 | |||
194 | // save now: we want the import on record before starting it |
||
195 | 4 | $this->getRepository()->save($import); |
|
196 | |||
197 | // add parts |
||
198 | try { |
||
199 | 4 | $this->addImportParts($import); |
|
200 | } catch (\Exception $e) { |
||
201 | // temporary store the exception, so we can update the import appropriately |
||
202 | $exception = $e; |
||
203 | } |
||
204 | } |
||
205 | |||
206 | // finish import right away if we have no parts, without parts it would never be started |
||
207 | 4 | if (count($import->getParts()) === 0) { |
|
208 | $this->getRepository()->startImport($import); |
||
209 | $this->getRepository()->finishImport($import); |
||
210 | } |
||
211 | |||
212 | // throw the exception, because catch all exceptions are evil |
||
213 | 4 | if ($exception) { |
|
214 | throw $exception; |
||
215 | } |
||
216 | |||
217 | 4 | return $import; |
|
218 | } |
||
219 | |||
220 | /** |
||
221 | * @param ImportPart $part |
||
222 | * @param EventDispatcherInterface $dispatcher |
||
223 | * |
||
224 | * @return ImportJob |
||
225 | */ |
||
226 | 4 | public function createImportJob(ImportPart $part, EventDispatcherInterface $dispatcher = null) |
|
239 | |||
240 | /** |
||
241 | * @param Import $import |
||
242 | * @param EventDispatcherInterface $dispatcher |
||
243 | * @param array $options |
||
244 | * |
||
245 | * @return Importer |
||
246 | */ |
||
247 | 4 | protected function createImporter(Import $import, EventDispatcherInterface $dispatcher, array $options = []) |
|
257 | |||
258 | /** |
||
259 | * @param Import $import |
||
260 | * @param array $transport |
||
261 | * @param \DateTime $scheduleDate |
||
262 | * @param int $position |
||
263 | * |
||
264 | * @return ImportPart |
||
265 | */ |
||
266 | 4 | protected function createImportPart(Import $import, array $transport, \DateTime $scheduleDate = null, $position = null) |
|
267 | { |
||
268 | 4 | if (is_null($scheduleDate)) { |
|
269 | 4 | $scheduleDate = new \DateTime(); |
|
270 | } |
||
271 | |||
272 | 4 | if (is_null($position)) { |
|
273 | $positions = $import |
||
274 | 4 | ->getParts() |
|
275 | 4 | ->map(function (ImportPart $part) { |
|
276 | return $part->getPosition(); |
||
277 | 4 | }) |
|
278 | 4 | ->toArray() |
|
279 | ; |
||
280 | |||
281 | // add this to ensure we have at least 1 position |
||
282 | 4 | $positions[] = 0; |
|
283 | |||
284 | 4 | $position = max($positions) + 1; |
|
285 | } |
||
286 | |||
287 | 4 | $part = new ImportPart(); |
|
288 | 4 | $part->setPosition($position); |
|
289 | 4 | $part->setTransportConfig($transport); |
|
290 | 4 | $part->setDatetimeScheduled($scheduleDate); |
|
291 | 4 | $part->setImport($import); |
|
292 | 4 | $import->addPart($part); |
|
293 | |||
294 | 4 | $this->getRepository()->savePart($part); |
|
295 | |||
296 | 4 | return $part; |
|
297 | } |
||
298 | |||
299 | /** |
||
300 | * @param Import $import |
||
301 | * @param array $transport |
||
302 | * @param string $resourceType |
||
303 | * @param EventDispatcherInterface $dispatcher |
||
304 | * @param array $options |
||
305 | * |
||
306 | * @return ReaderInterface |
||
307 | */ |
||
308 | 4 | protected function createReader(Import $import, array $transport, $resourceType, EventDispatcherInterface $dispatcher, array $options = []) |
|
319 | |||
320 | /** |
||
321 | * @param Import $import |
||
322 | * @param EventDispatcherInterface $dispatcher |
||
323 | * @param array $options |
||
324 | * |
||
325 | * @return ReaderInterface |
||
326 | */ |
||
327 | 4 | View Code Duplication | protected function createImportReader(Import $import, EventDispatcherInterface $dispatcher, array $options = []) |
335 | |||
336 | /** |
||
337 | * @param ImportPart $importPart |
||
338 | * @param EventDispatcherInterface $dispatcher |
||
339 | * @param array $options |
||
340 | * |
||
341 | * @return ReaderInterface |
||
342 | */ |
||
343 | 4 | View Code Duplication | protected function createImportPartReader(ImportPart $importPart, EventDispatcherInterface $dispatcher, array $options = []) |
344 | { |
||
345 | 4 | $import = $importPart->getImport(); |
|
346 | 4 | $transport = $importPart->getTransportConfig(); |
|
347 | 4 | $resourceType = ReaderBuilderInterface::RESOURCE_TYPE_PART; |
|
348 | |||
349 | 4 | return $this->createReader($import, $transport, $resourceType, $dispatcher, $options); |
|
350 | } |
||
351 | |||
352 | /** |
||
353 | * @param FeedEntity $feed |
||
354 | * @param ReaderInterface $reader |
||
355 | * @param EventDispatcherInterface $dispatcher |
||
356 | * @param array $options |
||
357 | * |
||
358 | * @return Feed |
||
359 | */ |
||
360 | 4 | protected function createFeed(FeedEntity $feed, ReaderInterface $reader, EventDispatcherInterface $dispatcher, array $options = []) |
|
368 | |||
369 | /** |
||
370 | * @return EventDispatcherInterface |
||
371 | */ |
||
372 | 4 | protected function createEventDispatcher() |
|
379 | |||
380 | /** |
||
381 | * @param FeedEntity $feed |
||
382 | * |
||
383 | * @throws UnfinishedImportException |
||
384 | * |
||
385 | * @return Import |
||
386 | */ |
||
387 | 4 | protected function findOrCreateImport(FeedEntity $feed) |
|
388 | { |
||
389 | /** @var $imports Import[] */ |
||
390 | 4 | $imports = $this->getRepository()->findBy(['feed' => $feed]); |
|
391 | |||
392 | 4 | foreach ($imports as $import) { |
|
393 | // skip finished imports |
||
394 | 2 | if ($import->isFinished()) { |
|
395 | 2 | continue; |
|
396 | } |
||
397 | |||
398 | // if it hasn't started yet, use this one |
||
399 | if (!$import->isStarted()) { |
||
400 | return $import; |
||
401 | } |
||
402 | |||
403 | try { |
||
404 | $this->getRepository()->finishImport($import); |
||
405 | } catch (UnfinishedImportException $e) { |
||
406 | throw new UnfinishedImportException( |
||
407 | $import, |
||
408 | sprintf( |
||
409 | 'Import %d has unfinished parts, close those first before creating a new import', |
||
410 | $import->getId() |
||
411 | ) |
||
412 | ); |
||
413 | } |
||
414 | } |
||
415 | |||
416 | // all previous imports are checked and finished if necessary, we can create a new one now |
||
417 | 4 | $import = new Import(); |
|
418 | 4 | $import->setFeed($feed); |
|
419 | |||
420 | 4 | return $import; |
|
421 | } |
||
422 | |||
423 | 4 | protected function addImportParts(Import $import) |
|
424 | { |
||
425 | 4 | $dispatcher = $this->createEventDispatcher(); |
|
426 | 4 | $options = $this->getDefaultReaderOptions($import); |
|
427 | |||
428 | 4 | $reader = $this->createImportReader($import, $dispatcher, $options); |
|
429 | |||
430 | 4 | foreach ($reader->getResources() as $resource) { |
|
431 | 4 | $transport = TransportFactory::createConfigFromTransport($resource->getTransport()); |
|
432 | |||
433 | 4 | $part = $this->createImportPart($import, $transport); |
|
434 | 4 | $this->eventDispatcher->dispatch(ImportEvents::PART_CREATED, new PartEvent($part)); |
|
435 | } |
||
436 | 4 | } |
|
437 | |||
438 | /** |
||
439 | * @return ImportRepository |
||
440 | */ |
||
441 | 4 | protected function getRepository() |
|
445 | |||
446 | /** |
||
447 | * @param Import $import |
||
448 | * |
||
449 | * @return HandlerInterface |
||
450 | */ |
||
451 | 4 | protected function getImportHandler(Import $import) |
|
455 | |||
456 | /** |
||
457 | * @param Import $import |
||
458 | * |
||
459 | * @return ProcessorInterface |
||
460 | */ |
||
461 | 4 | protected function getImportProcessor(Import $import) |
|
465 | |||
466 | /** |
||
467 | * Returns default options to pass to the feed builder. |
||
468 | * |
||
469 | * @param Import $import |
||
470 | * |
||
471 | * @return array |
||
472 | */ |
||
473 | 4 | protected function getDefaultFeedOptions(Import $import) |
|
474 | { |
||
475 | return [ |
||
476 | 4 | 'forced' => $import->isForced(), |
|
477 | 4 | 'feed' => $import->getFeed(), |
|
478 | 4 | 'default_values' => $import->getFeed()->getDefaultValues(), |
|
479 | ]; |
||
480 | } |
||
481 | |||
482 | /** |
||
483 | * Returns default options to pass to the reader builder. |
||
484 | * |
||
485 | * @param Import $import |
||
486 | * |
||
487 | * @return array |
||
488 | */ |
||
489 | 4 | protected function getDefaultReaderOptions(Import $import) |
|
490 | { |
||
491 | return [ |
||
492 | 4 | 'partial' => $import->isPartial(), |
|
493 | 4 | 'forced' => $import->isForced(), |
|
494 | ]; |
||
495 | } |
||
496 | |||
497 | /** |
||
498 | * Returns default options to pass to the importer builder. |
||
499 | * |
||
500 | * @param Import $import |
||
501 | * |
||
502 | * @return array |
||
503 | */ |
||
504 | 4 | protected function getDefaultImporterOptions(Import $import) |
|
508 | } |
||
509 |
Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.
You can also find more detailed suggestions in the “Code” section of your repository.