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 AbstractEmailSynchronizer 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 AbstractEmailSynchronizer, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
25 | abstract class AbstractEmailSynchronizer implements LoggerAwareInterface |
||
26 | { |
||
27 | use LoggerAwareTrait; |
||
28 | |||
29 | const SYNC_CODE_IN_PROCESS = 1; |
||
30 | const SYNC_CODE_FAILURE = 2; |
||
31 | const SYNC_CODE_SUCCESS = 3; |
||
32 | |||
33 | /** @var ManagerRegistry */ |
||
34 | protected $doctrine; |
||
35 | |||
36 | /** @var KnownEmailAddressCheckerFactory */ |
||
37 | protected $knownEmailAddressCheckerFactory; |
||
38 | |||
39 | /** @var TokenStorage */ |
||
40 | protected $tokenStorage; |
||
41 | |||
42 | /** @var KnownEmailAddressCheckerInterface */ |
||
43 | private $knownEmailAddressChecker; |
||
44 | |||
45 | /** @var TokenInterface */ |
||
46 | private $currentToken; |
||
47 | |||
48 | /** |
||
49 | * Constructor |
||
50 | * |
||
51 | * @param ManagerRegistry $doctrine |
||
52 | * @param KnownEmailAddressCheckerFactory $knownEmailAddressCheckerFactory |
||
53 | */ |
||
54 | protected function __construct( |
||
55 | ManagerRegistry $doctrine, |
||
56 | KnownEmailAddressCheckerFactory $knownEmailAddressCheckerFactory |
||
57 | ) { |
||
58 | $this->doctrine = $doctrine; |
||
59 | $this->knownEmailAddressCheckerFactory = $knownEmailAddressCheckerFactory; |
||
60 | } |
||
61 | |||
62 | /** |
||
63 | * @param TokenStorage $tokenStorage |
||
64 | */ |
||
65 | public function setTokenStorage(TokenStorage $tokenStorage) |
||
66 | { |
||
67 | $this->tokenStorage = $tokenStorage; |
||
68 | $this->currentToken = $tokenStorage->getToken(); |
||
69 | } |
||
70 | |||
71 | /** |
||
72 | * Returns TRUE if this class supports synchronization of the given origin. |
||
73 | * |
||
74 | * @param EmailOrigin $origin |
||
75 | * @return bool |
||
76 | */ |
||
77 | abstract public function supports(EmailOrigin $origin); |
||
78 | |||
79 | /** |
||
80 | * Performs a synchronization of emails for one email origin. |
||
81 | * Algorithm how an email origin is selected see in findOriginToSync method. |
||
82 | * |
||
83 | * @param int $maxConcurrentTasks The maximum number of synchronization jobs running in the same time |
||
84 | * @param int $minExecIntervalInMin The minimum time interval (in minutes) between two synchronizations |
||
85 | * of the same email origin |
||
86 | * @param int $maxExecTimeInMin The maximum execution time (in minutes) |
||
87 | * Set -1 to unlimited |
||
88 | * Defaults to -1 |
||
89 | * @param int $maxTasks The maximum number of email origins which can be synchronized |
||
90 | * Set -1 to unlimited |
||
91 | * Defaults to 1 |
||
92 | * @return int |
||
93 | * |
||
94 | * @throws \Exception |
||
95 | * |
||
96 | * @SuppressWarnings(PHPMD.NPathComplexity) |
||
97 | */ |
||
98 | public function sync($maxConcurrentTasks, $minExecIntervalInMin, $maxExecTimeInMin = -1, $maxTasks = 1) |
||
156 | |||
157 | /** |
||
158 | * Performs a synchronization of emails for the given email origins. |
||
159 | * |
||
160 | * @param int[] $originIds |
||
161 | * @throws \Exception |
||
162 | */ |
||
163 | public function syncOrigins(array $originIds) |
||
189 | |||
190 | /** |
||
191 | * Checks configuration |
||
192 | * This method can be used for preliminary check if the synchronization can be launched |
||
193 | * |
||
194 | * @return bool |
||
195 | */ |
||
196 | protected function checkConfiguration() |
||
200 | |||
201 | /** |
||
202 | * Performs a synchronization of emails for the given email origin. |
||
203 | * |
||
204 | * @param EmailOrigin $origin |
||
205 | * @throws \Exception |
||
206 | */ |
||
207 | protected function doSyncOrigin(EmailOrigin $origin) |
||
253 | |||
254 | /** |
||
255 | * Switches the security context to the given organization |
||
256 | * @todo: Should be deleted after email sync process will be refactored |
||
257 | */ |
||
258 | protected function impersonateOrganization(Organization $organization = null) |
||
266 | |||
267 | /** |
||
268 | * Returns default entity manager |
||
269 | * |
||
270 | * @return EntityManager |
||
271 | */ |
||
272 | View Code Duplication | protected function getEntityManager() |
|
283 | |||
284 | /** |
||
285 | * Makes sure $this->knownEmailAddressChecker initialized |
||
286 | */ |
||
287 | protected function getKnownEmailAddressChecker() |
||
298 | |||
299 | /** |
||
300 | * Gets entity name implementing EmailOrigin |
||
301 | * |
||
302 | * @return string |
||
303 | */ |
||
304 | abstract protected function getEmailOriginClass(); |
||
305 | |||
306 | /** |
||
307 | * Creates a processor is used to synchronize emails |
||
308 | * |
||
309 | * @param object $origin An instance of class implementing EmailOrigin entity |
||
310 | * @return AbstractEmailSynchronizationProcessor |
||
311 | */ |
||
312 | abstract protected function createSynchronizationProcessor($origin); |
||
313 | |||
314 | /** |
||
315 | * Updates a state of the given email origin |
||
316 | * |
||
317 | * @param EmailOrigin $origin |
||
318 | * @param int $syncCode Can be one of self::SYNC_CODE_* constants |
||
319 | * @param \DateTime|null $synchronizedAt |
||
320 | * @return bool true if the synchronization code was updated; false if no any changes are needed |
||
321 | */ |
||
322 | protected function changeOriginSyncState(EmailOrigin $origin, $syncCode, $synchronizedAt = null) |
||
352 | |||
353 | /** |
||
354 | * Finds an email origin to be synchronised |
||
355 | * |
||
356 | * @param int $maxConcurrentTasks The maximum number of synchronization jobs running in the same time |
||
357 | * @param int $minExecIntervalInMin The minimum time interval (in minutes) between two synchronizations |
||
358 | * of the same email origin |
||
359 | * @return EmailOrigin |
||
360 | */ |
||
361 | protected function findOriginToSync($maxConcurrentTasks, $minExecIntervalInMin) |
||
419 | |||
420 | /** |
||
421 | * Finds active email origin by its id |
||
422 | * |
||
423 | * @param int $originId |
||
424 | * @return EmailOrigin|null |
||
425 | */ |
||
426 | protected function findOrigin($originId) |
||
450 | |||
451 | /** |
||
452 | * Marks outdated "In Process" origins as "Failure" if exist |
||
453 | */ |
||
454 | protected function resetHangedOrigins() |
||
475 | |||
476 | /** |
||
477 | * Gets a DateTime object that is set to the current date and time in UTC. |
||
478 | * |
||
479 | * @return \DateTime |
||
480 | */ |
||
481 | protected function getCurrentUtcDateTime() |
||
485 | |||
486 | /** |
||
487 | * @param $failedOriginIds |
||
488 | * @throws \Exception |
||
489 | */ |
||
490 | private function assertSyncSuccess(array $failedOriginIds) |
||
501 | } |
||
502 |
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: