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 |
It seems like the type of the argument is not accepted by the function/method which you are calling.
In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.
We suggest to add an explicit type cast like in the following example: