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 Backup_Status 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 Backup_Status, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
10 | class Backup_Status { |
||
11 | |||
12 | /** |
||
13 | * The filename for the backup file we are the tracking status of. |
||
14 | * |
||
15 | * @var string |
||
16 | */ |
||
17 | private $filename = ''; |
||
18 | |||
19 | /** |
||
20 | * [$lock_handler description] |
||
21 | * |
||
22 | * @var LockHandler |
||
23 | */ |
||
24 | private $lock_handler = ''; |
||
25 | |||
26 | private $callback; |
||
27 | |||
28 | /** |
||
29 | * @param string $id The unique id for the backup job. |
||
30 | */ |
||
31 | public function __construct( $id ) { |
||
35 | |||
36 | /** |
||
37 | * Start the tracking a backup process. |
||
38 | * |
||
39 | * This creates a backup running file and issues a file lock. This prevents duplicate |
||
40 | * instances of this backup process running concurrently and allows us to detect if |
||
41 | * the PHP thread running the process is killed as that will clear the lock. |
||
42 | * |
||
43 | * @param string $backup_filename The filename for the backup file that we're tracking. |
||
44 | * @param string $status_message The initial status for the backup process. |
||
45 | * |
||
46 | * @return boolean Whether the backup process was success marked as started. |
||
47 | */ |
||
48 | public function start( $backup_filename, $status_message ) { |
||
66 | |||
67 | /** |
||
68 | * Mark a backup process as finished. |
||
69 | * |
||
70 | * This removes the file lock and deletes the running file. |
||
71 | */ |
||
72 | public function finish() { |
||
87 | |||
88 | /** |
||
89 | * Check if the backup has been started by checking if the running file |
||
90 | * exists. |
||
91 | * |
||
92 | * @return boolean Whether the backup process has been started. |
||
93 | */ |
||
94 | public function is_started() { |
||
97 | |||
98 | public function is_running() { |
||
119 | |||
120 | /** |
||
121 | * If the running file exists but isn't locked then the thread that |
||
122 | * the backup process is running in must have been killed. |
||
123 | * |
||
124 | * You should only be running this command from a separate thread. |
||
125 | * |
||
126 | * @return boolean Whether the backup process has crashed or not. |
||
127 | */ |
||
128 | public function has_crashed() { |
||
131 | |||
132 | /** |
||
133 | * Handle a process that's previously crashed. |
||
134 | * |
||
135 | * Delete the partially created backup if it exists and then |
||
136 | * run the standard cleanup tasks and set an error message for the user. |
||
137 | * |
||
138 | * @return bool Whether the crash was handled or not. |
||
139 | */ |
||
140 | public function cleanup_after_crash() { |
||
160 | |||
161 | /** |
||
162 | * Catch fatal errors and react accordingly. |
||
163 | * |
||
164 | * Hooked into the shutdown action. If we've shutdown because of a Fatal error |
||
165 | * then we cleanup and set an error message for the user. |
||
166 | */ |
||
167 | public function catch_fatals() { |
||
191 | |||
192 | /** |
||
193 | * Get the filepath for the backup file we're tracking. |
||
194 | * |
||
195 | * @return string The path to the backup file. |
||
196 | */ |
||
197 | public function get_backup_filename() { |
||
209 | |||
210 | /** |
||
211 | * Get the status of the running backup. |
||
212 | * |
||
213 | * @return string |
||
214 | */ |
||
215 | public function get_status() { |
||
229 | |||
230 | /** |
||
231 | * Set the status of the running backup. |
||
232 | * |
||
233 | * @param string $message |
||
234 | * |
||
235 | * @return null |
||
236 | */ |
||
237 | public function set_status( $message ) { |
||
256 | |||
257 | /** |
||
258 | * Get the time that the current running backup was started. |
||
259 | * |
||
260 | * @return int $timestamp |
||
261 | */ |
||
262 | public function get_start_time() { |
||
276 | |||
277 | /** |
||
278 | * Get the path to the backup running file that stores the running backup status. |
||
279 | * |
||
280 | * @return string |
||
281 | */ |
||
282 | public function get_status_filepath() { |
||
285 | |||
286 | public function set_status_callback( $callback ) { |
||
289 | } |
||
290 |
In PHP it is possible to write to properties without declaring them. For example, the following is perfectly valid PHP code:
Generally, it is a good practice to explictly declare properties to avoid accidental typos and provide IDE auto-completion: