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 UserConfirmationStep 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 UserConfirmationStep, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
55 | class UserConfirmationStep extends LongRunningPipelineStep { |
||
56 | |||
57 | /** |
||
58 | * Messages |
||
59 | */ |
||
60 | const ALERT_APPROVE = 'Approve'; |
||
61 | const ALERT_TIMEOUT = 'TimeOut'; |
||
62 | const ALERT_REQUEST = 'Request'; |
||
63 | const ALERT_REJECT = 'Reject'; |
||
64 | |||
65 | /** |
||
66 | * Message roles |
||
67 | */ |
||
68 | const ROLE_REQUESTER = 'Requester'; |
||
69 | const ROLE_RECIPIENT = 'Recipient'; |
||
70 | |||
71 | /** |
||
72 | * @var array |
||
73 | */ |
||
74 | private static $db = array( |
||
75 | // A finished step is approved and a failed step is rejected. |
||
76 | // Aborted confirmation is left as None |
||
77 | 'Approval' => "Enum('Approved,Rejected,None', 'None')", |
||
78 | // If RecipientsDelay is specified, this value records the index of the most recently notified |
||
79 | // group of users. This will be incremented once another level of fallback has been notified. |
||
80 | // E.g. once primary admin has been notified, the secondary admin can be notified, and this |
||
81 | // is incremented |
||
82 | 'NotifiedGroup' => 'Int' |
||
83 | ); |
||
84 | |||
85 | /** |
||
86 | * @var array |
||
87 | */ |
||
88 | private static $defaults = array( |
||
89 | 'Approval' => 'None', |
||
90 | 'NotifiedGroup' => 0 |
||
91 | ); |
||
92 | |||
93 | /** |
||
94 | * @var array |
||
95 | */ |
||
96 | private static $has_one = array( |
||
97 | 'Responder' => 'Member' |
||
98 | ); |
||
99 | |||
100 | /** |
||
101 | * This step depends on a configured messaging service |
||
102 | * |
||
103 | * @config |
||
104 | * @var array |
||
105 | */ |
||
106 | private static $dependencies = array( |
||
107 | 'MessagingService' => '%$ConfirmationMessagingService' |
||
108 | ); |
||
109 | |||
110 | /** |
||
111 | * Currently assigned messaging service |
||
112 | * |
||
113 | * @var ConfirmationMessagingService |
||
114 | */ |
||
115 | private $messagingService = null; |
||
116 | |||
117 | /** |
||
118 | * Assign a messaging service for this step |
||
119 | * |
||
120 | * @param ConfirmationMessagingService $service |
||
121 | */ |
||
122 | public function setMessagingService(ConfirmationMessagingService $service) { |
||
125 | |||
126 | /** |
||
127 | * Get the currently configured messaging service |
||
128 | * |
||
129 | * @return ConfirmationMessagingService |
||
130 | */ |
||
131 | public function getMessagingService() { |
||
134 | |||
135 | /** |
||
136 | * Determine if the confirmation has been responded to (ether with acceptance, rejection, or cancelled) |
||
137 | * |
||
138 | * @return boolean |
||
139 | */ |
||
140 | public function hasResponse() { |
||
143 | |||
144 | public function start() { |
||
145 | parent::start(); |
||
146 | |||
147 | // Just in case this step is being mistakenly restarted |
||
148 | if($this->hasResponse()) { |
||
149 | $this->log("{$this->Title} has already been processed with a response of {$this->Approval}"); |
||
150 | $this->markFailed(); |
||
151 | return false; |
||
152 | } |
||
153 | |||
154 | // Begin or process this step |
||
155 | switch($this->Status) { |
||
156 | case 'Started': |
||
157 | return $this->checkStatus(); |
||
158 | case 'Queued': |
||
159 | return $this->startApproval(); |
||
160 | default: |
||
161 | $this->log("Unable to process {$this->Title} with status of {$this->Status}"); |
||
162 | $this->markFailed(); |
||
163 | return false; |
||
164 | } |
||
165 | } |
||
166 | |||
167 | /** |
||
168 | * Can the current user approve this pipeline? |
||
169 | * |
||
170 | * @param Member|null $member |
||
171 | * @return boolean |
||
172 | */ |
||
173 | public function canApprove($member = null) { |
||
176 | |||
177 | /** |
||
178 | * When the recipient wishes to approve this request |
||
179 | * |
||
180 | * @return boolean True if successful |
||
181 | */ |
||
182 | View Code Duplication | public function approve() { |
|
204 | |||
205 | /** |
||
206 | * When the recipient wishes to reject this request |
||
207 | * |
||
208 | * @return boolean True if successful |
||
209 | */ |
||
210 | View Code Duplication | public function reject() { |
|
232 | |||
233 | /** |
||
234 | * Report the status of the current request queue and makes sure it hasn't overrun it's time allowed |
||
235 | * |
||
236 | * @return boolean True if not failed |
||
237 | */ |
||
238 | protected function checkStatus() { |
||
279 | |||
280 | /** |
||
281 | * Initiate the approval process |
||
282 | */ |
||
283 | public function startApproval() { |
||
296 | |||
297 | /** |
||
298 | * Finds a message template for a given role and message |
||
299 | * |
||
300 | * @param string $role Role name for role-customised messages. Usually 'Requester' or 'Recipient' |
||
301 | * @param string $messageID Message ID |
||
302 | * @return array Resulting array(subject, message) |
||
303 | */ |
||
304 | protected function generateMessageTemplate($role, $messageID) { |
||
314 | |||
315 | /** |
||
316 | * Retrieve message replacements |
||
317 | * |
||
318 | * @return array |
||
319 | */ |
||
320 | public function getReplacements() { |
||
331 | |||
332 | /** |
||
333 | * Sends a message to a specified recipient(s) |
||
334 | * |
||
335 | * @param string $messageID Message ID. One of 'Reject', 'Approve', 'TimeOut' or 'Request' |
||
336 | * @param mixed $recipientGroup Either a numeric index of the next recipient to send to, or "all" for all |
||
337 | * This is used for delayed notification so that failover recipients can be notified. |
||
338 | * @return boolean|null True if successful |
||
339 | */ |
||
340 | protected function sendMessage($messageID, $recipientGroup = 'all') { |
||
378 | |||
379 | public function getRunningDescription() { |
||
388 | |||
389 | public function allowedActions() { |
||
411 | |||
412 | } |
||
413 |
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.