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:
1 | <?php |
||
22 | class Request extends DataObject |
||
23 | { |
||
24 | private $email; |
||
25 | private $ip; |
||
26 | private $name; |
||
27 | /** @var string|null */ |
||
28 | private $comment; |
||
29 | private $status = "Open"; |
||
30 | private $date; |
||
31 | private $emailsent = 0; |
||
32 | private $emailconfirm; |
||
33 | /** @var int|null */ |
||
34 | private $reserved = null; |
||
35 | private $useragent; |
||
36 | private $forwardedip; |
||
37 | private $hasComments = false; |
||
38 | private $hasCommentsResolved = false; |
||
39 | |||
40 | /** |
||
41 | * @throws Exception |
||
42 | */ |
||
43 | public function save() |
||
44 | { |
||
45 | if ($this->isNew()) { |
||
46 | // insert |
||
47 | $statement = $this->dbObject->prepare(<<<SQL |
||
48 | INSERT INTO `request` ( |
||
49 | email, ip, name, comment, status, date, emailsent, |
||
50 | emailconfirm, reserved, useragent, forwardedip |
||
51 | ) VALUES ( |
||
52 | :email, :ip, :name, :comment, :status, CURRENT_TIMESTAMP(), :emailsent, |
||
53 | :emailconfirm, :reserved, :useragent, :forwardedip |
||
54 | ); |
||
55 | SQL |
||
56 | ); |
||
57 | $statement->bindValue(':email', $this->email); |
||
58 | $statement->bindValue(':ip', $this->ip); |
||
59 | $statement->bindValue(':name', $this->name); |
||
60 | $statement->bindValue(':comment', $this->comment); |
||
61 | $statement->bindValue(':status', $this->status); |
||
62 | $statement->bindValue(':emailsent', $this->emailsent); |
||
63 | $statement->bindValue(':emailconfirm', $this->emailconfirm); |
||
64 | $statement->bindValue(':reserved', $this->reserved); |
||
65 | $statement->bindValue(':useragent', $this->useragent); |
||
66 | $statement->bindValue(':forwardedip', $this->forwardedip); |
||
67 | |||
68 | if ($statement->execute()) { |
||
69 | $this->id = (int)$this->dbObject->lastInsertId(); |
||
70 | } |
||
71 | else { |
||
72 | throw new Exception($statement->errorInfo()); |
||
73 | } |
||
74 | } |
||
75 | else { |
||
76 | // update |
||
77 | $statement = $this->dbObject->prepare(<<<SQL |
||
78 | UPDATE `request` SET |
||
79 | status = :status, |
||
80 | emailsent = :emailsent, |
||
81 | emailconfirm = :emailconfirm, |
||
82 | reserved = :reserved, |
||
83 | updateversion = updateversion + 1 |
||
84 | WHERE id = :id AND updateversion = :updateversion |
||
85 | LIMIT 1; |
||
86 | SQL |
||
87 | ); |
||
88 | |||
89 | $statement->bindValue(':id', $this->id); |
||
90 | $statement->bindValue(':updateversion', $this->updateversion); |
||
91 | |||
92 | $statement->bindValue(':status', $this->status); |
||
93 | $statement->bindValue(':emailsent', $this->emailsent); |
||
94 | $statement->bindValue(':emailconfirm', $this->emailconfirm); |
||
95 | $statement->bindValue(':reserved', $this->reserved); |
||
96 | |||
97 | if (!$statement->execute()) { |
||
98 | throw new Exception($statement->errorInfo()); |
||
99 | } |
||
100 | |||
101 | if ($statement->rowCount() !== 1) { |
||
102 | throw new OptimisticLockFailedException(); |
||
103 | } |
||
104 | |||
105 | $this->updateversion++; |
||
106 | } |
||
107 | } |
||
108 | |||
109 | /** |
||
110 | * @return string |
||
111 | */ |
||
112 | public function getIp() |
||
113 | { |
||
114 | return $this->ip; |
||
115 | } |
||
116 | |||
117 | /** |
||
118 | * @param string $ip |
||
119 | */ |
||
120 | 1 | public function setIp($ip) |
|
124 | |||
125 | /** |
||
126 | * @return string |
||
127 | */ |
||
128 | 1 | public function getName() |
|
132 | |||
133 | /** |
||
134 | * @param string $name |
||
135 | */ |
||
136 | 1 | public function setName($name) |
|
140 | |||
141 | /** |
||
142 | * @return string|null |
||
143 | */ |
||
144 | public function getComment() |
||
148 | |||
149 | /** |
||
150 | * @param string $comment |
||
151 | */ |
||
152 | public function setComment($comment) |
||
156 | |||
157 | /** |
||
158 | * @return string |
||
159 | */ |
||
160 | public function getStatus() |
||
164 | |||
165 | /** |
||
166 | * @param string $status |
||
167 | */ |
||
168 | public function setStatus($status) |
||
172 | |||
173 | /** |
||
174 | * Returns the time the request was first submitted |
||
175 | * |
||
176 | * @return DateTimeImmutable |
||
177 | */ |
||
178 | public function getDate() |
||
179 | { |
||
180 | return new DateTimeImmutable($this->date); |
||
181 | } |
||
182 | |||
183 | /** |
||
184 | * @return bool |
||
185 | */ |
||
186 | public function getEmailSent() |
||
187 | { |
||
188 | return $this->emailsent == "1"; |
||
189 | } |
||
190 | |||
191 | /** |
||
192 | * @param bool $emailSent |
||
193 | */ |
||
194 | public function setEmailSent($emailSent) |
||
195 | { |
||
196 | $this->emailsent = $emailSent ? 1 : 0; |
||
197 | } |
||
198 | |||
199 | /** |
||
200 | * @return int|null |
||
201 | */ |
||
202 | public function getReserved() |
||
203 | { |
||
204 | return $this->reserved; |
||
205 | } |
||
206 | |||
207 | /** |
||
208 | * @param int|null $reserved |
||
209 | */ |
||
210 | public function setReserved($reserved) |
||
211 | { |
||
212 | $this->reserved = $reserved; |
||
213 | } |
||
214 | |||
215 | /** |
||
216 | * @return string |
||
217 | */ |
||
218 | public function getUserAgent() |
||
222 | |||
223 | /** |
||
224 | * @param string $useragent |
||
225 | */ |
||
226 | public function setUserAgent($useragent) |
||
230 | |||
231 | /** |
||
232 | * @return string|null |
||
233 | */ |
||
234 | public function getForwardedIp() |
||
238 | |||
239 | /** |
||
240 | * @param string|null $forwardedip |
||
241 | */ |
||
242 | public function setForwardedIp($forwardedip) |
||
246 | |||
247 | /** |
||
248 | * @return bool |
||
249 | */ |
||
250 | public function hasComments() |
||
273 | |||
274 | /** |
||
275 | * @return string |
||
276 | */ |
||
277 | public function getEmailConfirm() |
||
278 | { |
||
279 | return $this->emailconfirm; |
||
280 | } |
||
281 | |||
282 | /** |
||
283 | * @param string $emailconfirm |
||
284 | */ |
||
285 | public function setEmailConfirm($emailconfirm) |
||
286 | { |
||
287 | $this->emailconfirm = $emailconfirm; |
||
288 | } |
||
289 | |||
290 | public function generateEmailConfirmationHash() |
||
291 | { |
||
292 | $this->emailconfirm = bin2hex(openssl_random_pseudo_bytes(16)); |
||
293 | } |
||
294 | |||
295 | /** |
||
296 | * @return string|null |
||
297 | */ |
||
298 | 1 | public function getEmail() |
|
299 | { |
||
300 | 1 | return $this->email; |
|
301 | } |
||
302 | |||
303 | /** |
||
304 | * @param string|null $email |
||
305 | */ |
||
306 | 1 | public function setEmail($email) |
|
307 | { |
||
308 | 1 | $this->email = $email; |
|
309 | 1 | } |
|
310 | |||
311 | /** |
||
312 | * @return string |
||
313 | * @throws Exception |
||
314 | */ |
||
315 | public function getClosureReason() |
||
316 | { |
||
317 | if ($this->status != 'Closed') { |
||
318 | throw new Exception("Can't get closure reason for open request."); |
||
319 | } |
||
320 | |||
321 | $statement = $this->dbObject->prepare(<<<SQL |
||
322 | SELECT closes.mail_desc |
||
323 | FROM log |
||
324 | INNER JOIN closes ON log.action = closes.closes |
||
325 | WHERE log.objecttype = 'Request' |
||
326 | AND log.objectid = :requestId |
||
327 | AND log.action LIKE 'Closed%' |
||
328 | ORDER BY log.timestamp DESC |
||
329 | LIMIT 1; |
||
330 | SQL |
||
331 | ); |
||
332 | |||
333 | $statement->bindValue(":requestId", $this->id); |
||
334 | $statement->execute(); |
||
335 | $reason = $statement->fetchColumn(); |
||
336 | |||
337 | return $reason; |
||
338 | } |
||
339 | |||
340 | /** |
||
341 | * Gets a value indicating whether the request was closed as created or not. |
||
342 | */ |
||
343 | public function getWasCreated() |
||
344 | { |
||
345 | if ($this->status != 'Closed') { |
||
346 | throw new Exception("Can't get closure reason for open request."); |
||
347 | } |
||
348 | |||
349 | $statement = $this->dbObject->prepare(<<<SQL |
||
350 | SELECT emailtemplate.oncreated, log.action |
||
351 | FROM log |
||
352 | LEFT JOIN emailtemplate ON CONCAT('Closed ', emailtemplate.id) = log.action |
||
353 | WHERE log.objecttype = 'Request' |
||
354 | AND log.objectid = :requestId |
||
355 | AND log.action LIKE 'Closed%' |
||
356 | ORDER BY log.timestamp DESC |
||
357 | LIMIT 1; |
||
358 | SQL |
||
359 | ); |
||
360 | |||
361 | $statement->bindValue(":requestId", $this->id); |
||
362 | $statement->execute(); |
||
363 | $onCreated = $statement->fetchColumn(0); |
||
364 | $logAction = $statement->fetchColumn(1); |
||
365 | $statement->closeCursor(); |
||
366 | |||
367 | if ($onCreated === null) { |
||
368 | return $logAction === "Closed custom-y"; |
||
369 | } |
||
370 | |||
371 | return (bool)$onCreated; |
||
372 | } |
||
373 | |||
374 | /** |
||
375 | * @return DateTime |
||
376 | */ |
||
377 | View Code Duplication | public function getClosureDate() |
|
392 | |||
393 | /** |
||
394 | * Returns a hash based on data within this request which can be generated easily from the data to be used to reveal |
||
395 | * data to unauthorised* users. |
||
396 | * |
||
397 | * *:Not tool admins, check users, or the reserving user. |
||
398 | * |
||
399 | * @return string |
||
400 | * |
||
401 | * @todo future work to make invalidation better. Possibly move to the database and invalidate on relevant events? |
||
402 | * Maybe depend on the last logged action timestamp? |
||
403 | */ |
||
404 | public function getRevealHash() |
||
415 | } |
||
416 |
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.