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 Box 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 Box, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
12 | class Box |
||
13 | { |
||
14 | |||
15 | const BOX_STATUS_OPEN = 'OPEN'; |
||
16 | const BOX_STATUS_PENDING = 'PENDING'; |
||
17 | const BOX_STATUS_PRINTED = 'PRINTED'; |
||
18 | const BOX_STATUS_CANCELLED = 'CANCELLED'; |
||
19 | const BOX_STATUS_ON_HOLD = 'ON-HOLD'; |
||
20 | const BOX_STATUS_ANNOUNCED = 'ANNOUNCED'; |
||
21 | const BOX_STATUS_IN_TRANSIT = 'IN_TRANSIT'; |
||
22 | const BOX_STATUS_AWAITING_PICKUP = 'AWAITING_PICKUP'; |
||
23 | const BOX_STATUS_DELIVERED = 'DELIVERED'; |
||
24 | const BOX_STATUS_BACK_TO_SENDER = 'BACK_TO_SENDER'; |
||
25 | |||
26 | /** |
||
27 | * @var \Bpost\BpostApiClient\Bpost\Order\Sender |
||
28 | */ |
||
29 | private $sender; |
||
30 | |||
31 | /** |
||
32 | * @var \Bpost\BpostApiClient\Bpost\Order\Box\AtHome |
||
33 | */ |
||
34 | private $nationalBox; |
||
35 | |||
36 | /** |
||
37 | * @var \Bpost\BpostApiClient\Bpost\Order\Box\International |
||
38 | */ |
||
39 | private $internationalBox; |
||
40 | |||
41 | /** |
||
42 | * @var string |
||
43 | */ |
||
44 | private $remark; |
||
45 | |||
46 | /** |
||
47 | * @var string |
||
48 | */ |
||
49 | private $status; |
||
50 | |||
51 | /** @var string */ |
||
52 | private $barcode; |
||
53 | |||
54 | /** @var string */ |
||
55 | private $additionalCustomerReference; |
||
56 | |||
57 | /** |
||
58 | * @param \Bpost\BpostApiClient\Bpost\Order\Box\International $internationalBox |
||
59 | */ |
||
60 | 1 | public function setInternationalBox(Box\International $internationalBox) |
|
61 | { |
||
62 | 1 | $this->internationalBox = $internationalBox; |
|
63 | 1 | } |
|
64 | |||
65 | /** |
||
66 | * @return \Bpost\BpostApiClient\Bpost\Order\Box\International |
||
67 | */ |
||
68 | 4 | public function getInternationalBox() |
|
69 | { |
||
70 | 4 | return $this->internationalBox; |
|
71 | } |
||
72 | |||
73 | /** |
||
74 | * @param \Bpost\BpostApiClient\Bpost\Order\Box\National $nationalBox |
||
75 | */ |
||
76 | 3 | public function setNationalBox(Box\National $nationalBox) |
|
77 | { |
||
78 | 3 | $this->nationalBox = $nationalBox; |
|
|
|||
79 | 3 | } |
|
80 | |||
81 | /** |
||
82 | * @return \Bpost\BpostApiClient\Bpost\Order\Box\National |
||
83 | */ |
||
84 | 4 | public function getNationalBox() |
|
85 | { |
||
86 | 4 | return $this->nationalBox; |
|
87 | } |
||
88 | |||
89 | /** |
||
90 | * @param string $remark |
||
91 | */ |
||
92 | 4 | public function setRemark($remark) |
|
93 | { |
||
94 | 4 | $this->remark = $remark; |
|
95 | 4 | } |
|
96 | |||
97 | /** |
||
98 | * @return string |
||
99 | */ |
||
100 | 4 | public function getRemark() |
|
101 | { |
||
102 | 4 | return $this->remark; |
|
103 | } |
||
104 | |||
105 | /** |
||
106 | * @param \Bpost\BpostApiClient\Bpost\Order\Sender $sender |
||
107 | */ |
||
108 | 4 | public function setSender(Sender $sender) |
|
109 | { |
||
110 | 4 | $this->sender = $sender; |
|
111 | 4 | } |
|
112 | |||
113 | /** |
||
114 | * @return \Bpost\BpostApiClient\Bpost\Order\Sender |
||
115 | */ |
||
116 | 4 | public function getSender() |
|
117 | { |
||
118 | 4 | return $this->sender; |
|
119 | } |
||
120 | |||
121 | /** |
||
122 | * @param string $status |
||
123 | * @throws BpostInvalidValueException |
||
124 | */ |
||
125 | 2 | View Code Duplication | public function setStatus($status) |
126 | { |
||
127 | 2 | $status = strtoupper($status); |
|
128 | 2 | if (!in_array($status, self::getPossibleStatusValues())) { |
|
129 | 1 | throw new BpostInvalidValueException('status', $status, self::getPossibleStatusValues()); |
|
130 | } |
||
131 | |||
132 | 1 | $this->status = $status; |
|
133 | 1 | } |
|
134 | |||
135 | /** |
||
136 | * @param string $barcode |
||
137 | */ |
||
138 | 2 | public function setBarcode($barcode) |
|
139 | { |
||
140 | 2 | $this->barcode = strtoupper((string) $barcode); |
|
141 | 2 | } |
|
142 | |||
143 | /** |
||
144 | * @return string |
||
145 | */ |
||
146 | 3 | public function getBarcode() |
|
147 | { |
||
148 | 3 | return $this->barcode; |
|
149 | } |
||
150 | |||
151 | /** |
||
152 | * @return string |
||
153 | */ |
||
154 | 1 | public function getStatus() |
|
155 | { |
||
156 | 1 | return $this->status; |
|
157 | } |
||
158 | |||
159 | /** |
||
160 | * @param string $additionalCustomerReference |
||
161 | */ |
||
162 | 2 | public function setAdditionalCustomerReference($additionalCustomerReference) |
|
163 | { |
||
164 | 2 | $this->additionalCustomerReference = (string)$additionalCustomerReference; |
|
165 | 2 | } |
|
166 | |||
167 | /** |
||
168 | * @return string |
||
169 | */ |
||
170 | 4 | public function getAdditionalCustomerReference() |
|
171 | { |
||
172 | 4 | return $this->additionalCustomerReference; |
|
173 | } |
||
174 | |||
175 | /** |
||
176 | * @return array |
||
177 | */ |
||
178 | 2 | View Code Duplication | public static function getPossibleStatusValues() |
179 | { |
||
180 | return array( |
||
181 | 2 | self::BOX_STATUS_OPEN, |
|
182 | 2 | self::BOX_STATUS_PENDING, |
|
183 | 2 | self::BOX_STATUS_PRINTED, |
|
184 | 2 | self::BOX_STATUS_CANCELLED, |
|
185 | 2 | self::BOX_STATUS_ON_HOLD, |
|
186 | 2 | self::BOX_STATUS_ANNOUNCED, |
|
187 | 2 | self::BOX_STATUS_IN_TRANSIT, |
|
188 | 2 | self::BOX_STATUS_AWAITING_PICKUP, |
|
189 | 2 | self::BOX_STATUS_DELIVERED, |
|
190 | 2 | self::BOX_STATUS_BACK_TO_SENDER, |
|
191 | 2 | ); |
|
192 | } |
||
193 | |||
194 | /** |
||
195 | * Return the object as an array for usage in the XML |
||
196 | * |
||
197 | * @param \DomDocument $document |
||
198 | * @param string $prefix |
||
199 | * @return \DomElement |
||
200 | */ |
||
201 | 3 | public function toXML(\DOMDocument $document, $prefix = null) |
|
202 | { |
||
203 | 3 | $tagName = 'box'; |
|
204 | 3 | if ($prefix !== null) { |
|
205 | 1 | $tagName = $prefix . ':' . $tagName; |
|
206 | 1 | } |
|
207 | |||
208 | 3 | $box = $document->createElement($tagName); |
|
209 | |||
210 | 3 | if ($this->getSender() !== null) { |
|
211 | 3 | $box->appendChild( |
|
212 | 3 | $this->getSender()->toXML($document, $prefix) |
|
213 | 3 | ); |
|
214 | 3 | } |
|
215 | 3 | if ($this->getNationalBox() !== null) { |
|
216 | 2 | $box->appendChild( |
|
217 | 2 | $this->getNationalBox()->toXML($document, $prefix) |
|
218 | 2 | ); |
|
219 | 2 | } |
|
220 | 3 | if ($this->getInternationalBox() !== null) { |
|
221 | 1 | $box->appendChild( |
|
222 | 1 | $this->getInternationalBox()->toXML($document, $prefix) |
|
223 | 1 | ); |
|
224 | 1 | } |
|
225 | 3 | if ($this->getRemark() !== null) { |
|
226 | 3 | $tagName = 'remark'; |
|
227 | 3 | if ($prefix !== null) { |
|
228 | 1 | $tagName = $prefix . ':' . $tagName; |
|
229 | 1 | } |
|
230 | 3 | $box->appendChild( |
|
231 | 3 | $document->createElement( |
|
232 | 3 | $tagName, |
|
233 | 3 | $this->getRemark() |
|
234 | 3 | ) |
|
235 | 3 | ); |
|
236 | 3 | } |
|
237 | 3 | if ($this->getAdditionalCustomerReference() !== null) { |
|
238 | 1 | $tagName = 'additionalCustomerReference'; |
|
239 | 1 | if ($prefix !== null) { |
|
240 | 1 | $tagName = $prefix . ':' . $tagName; |
|
241 | 1 | } |
|
242 | 1 | $box->appendChild( |
|
243 | 1 | $document->createElement( |
|
244 | 1 | $tagName, |
|
245 | 1 | $this->getAdditionalCustomerReference() |
|
246 | 1 | ) |
|
247 | 1 | ); |
|
248 | 1 | } |
|
249 | 3 | if ($this->getBarcode() !== null) { |
|
250 | 2 | $tagName = 'barcode'; |
|
251 | 2 | if ($prefix !== null) { |
|
252 | $tagName = $prefix . ':' . $tagName; |
||
253 | } |
||
254 | 2 | $box->appendChild( |
|
255 | 2 | $document->createElement( |
|
256 | 2 | $tagName, |
|
257 | 2 | $this->getBarcode() |
|
258 | 2 | ) |
|
259 | 2 | ); |
|
260 | 2 | } |
|
261 | |||
262 | 3 | return $box; |
|
263 | } |
||
264 | |||
265 | /** |
||
266 | * @param \SimpleXMLElement $xml |
||
267 | * |
||
268 | * @return Box |
||
269 | * @throws BpostInvalidValueException |
||
270 | * @throws BpostNotImplementedException |
||
271 | */ |
||
272 | 1 | public static function createFromXML(\SimpleXMLElement $xml) |
|
339 | } |
||
340 |
Our type inference engine has found a suspicous assignment of a value to a property. This check raises an issue when a value that can be of a given class or a super-class is assigned to a property that is type hinted more strictly.
Either this assignment is in error or an instanceof check should be added for that assignment.