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 Job 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 Job, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
31 | abstract class Job implements IJobSpecification { |
||
32 | /** @var string */ |
||
33 | public $command; |
||
34 | |||
35 | /** @var array Array of job parameters */ |
||
36 | public $params; |
||
37 | |||
38 | /** @var array Additional queue metadata */ |
||
39 | public $metadata = []; |
||
40 | |||
41 | /** @var Title */ |
||
42 | protected $title; |
||
43 | |||
44 | /** @var bool Expensive jobs may set this to true */ |
||
45 | protected $removeDuplicates; |
||
46 | |||
47 | /** @var string Text for error that occurred last */ |
||
48 | protected $error; |
||
49 | |||
50 | /** @var callable[] */ |
||
51 | protected $teardownCallbacks = []; |
||
52 | |||
53 | /** |
||
54 | * Run the job |
||
55 | * @return bool Success |
||
56 | */ |
||
57 | abstract public function run(); |
||
58 | |||
59 | /** |
||
60 | * Create the appropriate object to handle a specific job |
||
61 | * |
||
62 | * @param string $command Job command |
||
63 | * @param Title $title Associated title |
||
64 | * @param array $params Job parameters |
||
65 | * @throws MWException |
||
66 | * @return Job |
||
67 | */ |
||
68 | public static function factory( $command, Title $title, $params = [] ) { |
||
82 | |||
83 | /** |
||
84 | * @param string $command |
||
85 | * @param Title $title |
||
86 | * @param array|bool $params Can not be === true |
||
87 | */ |
||
88 | public function __construct( $command, $title, $params = false ) { |
||
100 | |||
101 | /** |
||
102 | * Batch-insert a group of jobs into the queue. |
||
103 | * This will be wrapped in a transaction with a forced commit. |
||
104 | * |
||
105 | * This may add duplicate at insert time, but they will be |
||
106 | * removed later on, when the first one is popped. |
||
107 | * |
||
108 | * @param Job[] $jobs Array of Job objects |
||
109 | * @return bool |
||
110 | * @deprecated since 1.21 |
||
111 | */ |
||
112 | public static function batchInsert( $jobs ) { |
||
117 | |||
118 | /** |
||
119 | * @return string |
||
120 | */ |
||
121 | public function getType() { |
||
124 | |||
125 | /** |
||
126 | * @return Title |
||
127 | */ |
||
128 | public function getTitle() { |
||
131 | |||
132 | /** |
||
133 | * @return array |
||
134 | */ |
||
135 | public function getParams() { |
||
138 | |||
139 | /** |
||
140 | * @return int|null UNIX timestamp to delay running this job until, otherwise null |
||
141 | * @since 1.22 |
||
142 | */ |
||
143 | public function getReleaseTimestamp() { |
||
148 | |||
149 | /** |
||
150 | * @return int|null UNIX timestamp of when the job was queued, or null |
||
151 | * @since 1.26 |
||
152 | */ |
||
153 | public function getQueuedTimestamp() { |
||
158 | |||
159 | /** |
||
160 | * @return string|null Id of the request that created this job. Follows |
||
161 | * jobs recursively, allowing to track the id of the request that started a |
||
162 | * job when jobs insert jobs which insert other jobs. |
||
163 | * @since 1.27 |
||
164 | */ |
||
165 | public function getRequestId() { |
||
170 | |||
171 | /** |
||
172 | * @return int|null UNIX timestamp of when the job was runnable, or null |
||
173 | * @since 1.26 |
||
174 | */ |
||
175 | public function getReadyTimestamp() { |
||
178 | |||
179 | /** |
||
180 | * Whether the queue should reject insertion of this job if a duplicate exists |
||
181 | * |
||
182 | * This can be used to avoid duplicated effort or combined with delayed jobs to |
||
183 | * coalesce updates into larger batches. Claimed jobs are never treated as |
||
184 | * duplicates of new jobs, and some queues may allow a few duplicates due to |
||
185 | * network partitions and fail-over. Thus, additional locking is needed to |
||
186 | * enforce mutual exclusion if this is really needed. |
||
187 | * |
||
188 | * @return bool |
||
189 | */ |
||
190 | public function ignoreDuplicates() { |
||
193 | |||
194 | /** |
||
195 | * @return bool Whether this job can be retried on failure by job runners |
||
196 | * @since 1.21 |
||
197 | */ |
||
198 | public function allowRetries() { |
||
201 | |||
202 | /** |
||
203 | * @return int Number of actually "work items" handled in this job |
||
204 | * @see $wgJobBackoffThrottling |
||
205 | * @since 1.23 |
||
206 | */ |
||
207 | public function workItemCount() { |
||
210 | |||
211 | /** |
||
212 | * Subclasses may need to override this to make duplication detection work. |
||
213 | * The resulting map conveys everything that makes the job unique. This is |
||
214 | * only checked if ignoreDuplicates() returns true, meaning that duplicate |
||
215 | * jobs are supposed to be ignored. |
||
216 | * |
||
217 | * @return array Map of key/values |
||
218 | * @since 1.21 |
||
219 | */ |
||
220 | public function getDeduplicationInfo() { |
||
241 | |||
242 | /** |
||
243 | * Get "root job" parameters for a task |
||
244 | * |
||
245 | * This is used to no-op redundant jobs, including child jobs of jobs, |
||
246 | * as long as the children inherit the root job parameters. When a job |
||
247 | * with root job parameters and "rootJobIsSelf" set is pushed, the |
||
248 | * deduplicateRootJob() method is automatically called on it. If the |
||
249 | * root job is only virtual and not actually pushed (e.g. the sub-jobs |
||
250 | * are inserted directly), then call deduplicateRootJob() directly. |
||
251 | * |
||
252 | * @see JobQueue::deduplicateRootJob() |
||
253 | * |
||
254 | * @param string $key A key that identifies the task |
||
255 | * @return array Map of: |
||
256 | * - rootJobIsSelf : true |
||
257 | * - rootJobSignature : hash (e.g. SHA1) that identifies the task |
||
258 | * - rootJobTimestamp : TS_MW timestamp of this instance of the task |
||
259 | * @since 1.21 |
||
260 | */ |
||
261 | public static function newRootJobParams( $key ) { |
||
268 | |||
269 | /** |
||
270 | * @see JobQueue::deduplicateRootJob() |
||
271 | * @return array |
||
272 | * @since 1.21 |
||
273 | */ |
||
274 | View Code Duplication | public function getRootJobParams() { |
|
284 | |||
285 | /** |
||
286 | * @see JobQueue::deduplicateRootJob() |
||
287 | * @return bool |
||
288 | * @since 1.22 |
||
289 | */ |
||
290 | public function hasRootJobParams() { |
||
294 | |||
295 | /** |
||
296 | * @see JobQueue::deduplicateRootJob() |
||
297 | * @return bool Whether this is job is a root job |
||
298 | */ |
||
299 | public function isRootJob() { |
||
302 | |||
303 | /** |
||
304 | * @param callable $callback |
||
305 | * @since 1.27 |
||
306 | */ |
||
307 | protected function addTeardownCallback( $callback ) { |
||
310 | |||
311 | /** |
||
312 | * Do any final cleanup after run(), deferred updates, and all DB commits happen |
||
313 | * |
||
314 | * @since 1.27 |
||
315 | */ |
||
316 | public function teardown() { |
||
321 | |||
322 | /** |
||
323 | * Insert a single job into the queue. |
||
324 | * @return bool True on success |
||
325 | * @deprecated since 1.21 |
||
326 | */ |
||
327 | public function insert() { |
||
331 | |||
332 | /** |
||
333 | * @return string |
||
334 | */ |
||
335 | public function toString() { |
||
390 | |||
391 | protected function setLastError( $error ) { |
||
394 | |||
395 | public function getLastError() { |
||
398 | } |
||
399 |
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.