owncloud /
core
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 Sharing 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 Sharing, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 10 | trait Sharing { |
||
| 11 | use Provisioning; |
||
| 12 | |||
| 13 | /** @var int */ |
||
| 14 | private $sharingApiVersion = 1; |
||
| 15 | |||
| 16 | /** @var SimpleXMLElement */ |
||
| 17 | private $lastShareData = null; |
||
| 18 | |||
| 19 | /** @var int */ |
||
| 20 | private $savedShareId = null; |
||
| 21 | |||
| 22 | /** |
||
| 23 | * @Given /^as "([^"]*)" creating a share with$/ |
||
| 24 | * @param string $user |
||
| 25 | * @param \Behat\Gherkin\Node\TableNode|null $body |
||
| 26 | */ |
||
| 27 | public function asCreatingAShareWith($user, $body) { |
||
| 54 | |||
| 55 | /** |
||
| 56 | * @When /^creating a share with$/ |
||
| 57 | * @param \Behat\Gherkin\Node\TableNode|null $body |
||
| 58 | */ |
||
| 59 | public function creatingShare($body) { |
||
| 62 | |||
| 63 | /** |
||
| 64 | * @Then /^Public shared file "([^"]*)" can be downloaded$/ |
||
| 65 | */ |
||
| 66 | public function checkPublicSharedFile($filename) { |
||
| 85 | |||
| 86 | /** |
||
| 87 | * @Then /^Public shared file "([^"]*)" with password "([^"]*)" can be downloaded$/ |
||
| 88 | */ |
||
| 89 | public function checkPublicSharedFileWithPassword($filename, $password) { |
||
| 110 | |||
| 111 | /** |
||
| 112 | * @When /^Adding expiration date to last share$/ |
||
| 113 | */ |
||
| 114 | public function addingExpirationDate() { |
||
| 129 | |||
| 130 | /** |
||
| 131 | * @When /^Updating last share with$/ |
||
| 132 | * @param \Behat\Gherkin\Node\TableNode|null $body |
||
| 133 | */ |
||
| 134 | public function updatingLastShare($body) { |
||
| 162 | |||
| 163 | public function createShare($user, |
||
| 208 | |||
| 209 | public function isFieldInResponse($field, $contentExpected){ |
||
| 210 | $data = $this->response->xml()->data[0]; |
||
| 211 | if ((string)$field == 'expiration'){ |
||
| 212 | $contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00"; |
||
| 213 | } |
||
| 214 | |||
| 215 | if (count($data->element) > 0){ |
||
| 216 | foreach($data as $element) { |
||
| 217 | View Code Duplication | if ($contentExpected == "A_TOKEN"){ |
|
| 218 | return (strlen((string)$element->$field) == 15); |
||
| 219 | } |
||
| 220 | elseif ($contentExpected == "A_NUMBER"){ |
||
| 221 | return is_numeric((string)$element->$field); |
||
| 222 | } |
||
| 223 | elseif($contentExpected == "AN_URL"){ |
||
| 224 | return $this->isExpectedUrl((string)$element->$field, "index.php/s/"); |
||
| 225 | } |
||
| 226 | elseif ((string)$element->$field == $contentExpected){ |
||
| 227 | return True; |
||
| 228 | } |
||
| 229 | else{ |
||
| 230 | print($element->$field); |
||
| 231 | } |
||
| 232 | } |
||
| 233 | |||
| 234 | return False; |
||
| 235 | } else { |
||
| 236 | View Code Duplication | if ($contentExpected == "A_TOKEN"){ |
|
| 237 | return (strlen((string)$data->$field) == 15); |
||
| 238 | } |
||
| 239 | elseif ($contentExpected == "A_NUMBER"){ |
||
| 240 | return is_numeric((string)$data->$field); |
||
| 241 | } |
||
| 242 | elseif($contentExpected == "AN_URL"){ |
||
| 243 | return $this->isExpectedUrl((string)$data->$field, "index.php/s/"); |
||
| 244 | } |
||
| 245 | elseif ($data->$field == $contentExpected){ |
||
| 246 | return True; |
||
| 247 | } |
||
| 248 | return False; |
||
| 249 | } |
||
| 250 | } |
||
| 251 | |||
| 252 | /** |
||
| 253 | * @Then /^File "([^"]*)" should be included in the response$/ |
||
| 254 | * |
||
| 255 | * @param string $filename |
||
| 256 | */ |
||
| 257 | public function checkSharedFileInResponse($filename){ |
||
| 260 | |||
| 261 | /** |
||
| 262 | * @Then /^File "([^"]*)" should not be included in the response$/ |
||
| 263 | * |
||
| 264 | * @param string $filename |
||
| 265 | */ |
||
| 266 | public function checkSharedFileNotInResponse($filename){ |
||
| 269 | |||
| 270 | /** |
||
| 271 | * @Then /^User "([^"]*)" should be included in the response$/ |
||
| 272 | * |
||
| 273 | * @param string $user |
||
| 274 | */ |
||
| 275 | public function checkSharedUserInResponse($user){ |
||
| 278 | |||
| 279 | /** |
||
| 280 | * @Then /^User "([^"]*)" should not be included in the response$/ |
||
| 281 | * |
||
| 282 | * @param string $user |
||
| 283 | */ |
||
| 284 | public function checkSharedUserNotInResponse($user){ |
||
| 287 | |||
| 288 | public function isUserOrGroupInSharedData($userOrGroup){ |
||
| 297 | |||
| 298 | /** |
||
| 299 | * @Given /^file "([^"]*)" of user "([^"]*)" is shared with user "([^"]*)"$/ |
||
| 300 | * |
||
| 301 | * @param string $filepath |
||
| 302 | * @param string $user1 |
||
| 303 | * @param string $user2 |
||
| 304 | */ |
||
| 305 | View Code Duplication | public function assureFileIsShared($filepath, $user1, $user2){ |
|
| 323 | |||
| 324 | /** |
||
| 325 | * @Given /^file "([^"]*)" of user "([^"]*)" is shared with group "([^"]*)"$/ |
||
| 326 | * |
||
| 327 | * @param string $filepath |
||
| 328 | * @param string $user |
||
| 329 | * @param string $group |
||
| 330 | */ |
||
| 331 | View Code Duplication | public function assureFileIsSharedWithGroup($filepath, $user, $group){ |
|
| 349 | |||
| 350 | /** |
||
| 351 | * @When /^Deleting last share$/ |
||
| 352 | */ |
||
| 353 | public function deletingLastShare(){ |
||
| 358 | |||
| 359 | /** |
||
| 360 | * @When /^Getting info of last share$/ |
||
| 361 | */ |
||
| 362 | public function gettingInfoOfLastShare(){ |
||
| 367 | |||
| 368 | /** |
||
| 369 | * @Then /^last share_id is included in the answer$/ |
||
| 370 | */ |
||
| 371 | public function checkingLastShareIDIsIncluded(){ |
||
| 377 | |||
| 378 | /** |
||
| 379 | * @Then /^last share_id is included in the answer as parent$/ |
||
| 380 | */ |
||
| 381 | View Code Duplication | public function checkingLastShareIDIsIncludedAsParent(){ |
|
| 382 | $share_id = $this->lastShareData->data[0]->id; |
||
| 383 | //This is a problem before 9.0, setupFS is called with every api call so a new share id is created |
||
| 384 | //we just need to check the parent id to match with the returned id. |
||
| 385 | if (!$this->isFieldInResponse('parent', $share_id)){ |
||
| 386 | PHPUnit_Framework_Assert::fail("Share id $share_id not found in response as parent"); |
||
| 387 | } |
||
| 388 | } |
||
| 389 | |||
| 390 | /** |
||
| 391 | * @Then /^last share_id is not included in the answer$/ |
||
| 392 | */ |
||
| 393 | public function checkingLastShareIDIsNotIncluded(){ |
||
| 399 | |||
| 400 | /** |
||
| 401 | * @Then /^last share_id is not included in the answer as parent$/ |
||
| 402 | */ |
||
| 403 | View Code Duplication | public function checkingLastShareIDIsNotIncludedAsParent(){ |
|
| 404 | $share_id = $this->lastShareData->data[0]->id; |
||
| 405 | //This is a problem before 9.0, setupFS is called with every api call so a new share id is created |
||
| 406 | //we just need to check the parent id to match with the returned id. |
||
| 407 | if ($this->isFieldInResponse('parent', $share_id)){ |
||
| 408 | PHPUnit_Framework_Assert::fail("Share id $share_id has been found in response as parent"); |
||
| 409 | } |
||
| 410 | } |
||
| 411 | |||
| 412 | /** |
||
| 413 | * @Then /^Share fields of last share match with$/ |
||
| 414 | * @param \Behat\Gherkin\Node\TableNode|null $body |
||
| 415 | */ |
||
| 416 | public function checkShareFields($body){ |
||
| 435 | |||
| 436 | /** |
||
| 437 | * @Then As :user remove all shares from the file named :fileName |
||
| 438 | */ |
||
| 439 | public function asRemoveAllSharesFromTheFileNamed($user, $fileName) { |
||
| 479 | |||
| 480 | /** |
||
| 481 | * @When save last share id |
||
| 482 | */ |
||
| 483 | public function saveLastShareId() |
||
| 487 | |||
| 488 | /** |
||
| 489 | * @Then share ids should match |
||
| 490 | */ |
||
| 491 | public function shareIdsShouldMatch() |
||
| 497 | } |
||
| 498 | |||
| 499 |
Loading history...
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: