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 |
||
| 33 | class CardService { |
||
| 34 | |||
| 35 | private $cardMapper; |
||
| 36 | |||
| 37 | public function __construct(CardMapper $cardMapper, StackMapper $stackMapper, PermissionService $permissionService) { |
||
| 38 | $this->cardMapper = $cardMapper; |
||
| 39 | $this->stackMapper = $stackMapper; |
||
|
|
|||
| 40 | $this->permissionService = $permissionService; |
||
| 41 | } |
||
| 42 | |||
| 43 | public function find($cardId) { |
||
| 44 | $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); |
||
| 45 | return $this->cardMapper->find($cardId); |
||
| 46 | } |
||
| 47 | |||
| 48 | /** |
||
| 49 | * @param integer $order |
||
| 50 | */ |
||
| 51 | public function create($title, $stackId, $type, $order, $owner) { |
||
| 52 | $this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT); |
||
| 53 | $card = new Card(); |
||
| 54 | $card->setTitle($title); |
||
| 55 | $card->setStackId($stackId); |
||
| 56 | $card->setType($type); |
||
| 57 | $card->setOrder($order); |
||
| 58 | $card->setOwner($owner); |
||
| 59 | return $this->cardMapper->insert($card); |
||
| 60 | |||
| 61 | } |
||
| 62 | |||
| 63 | public function delete($id) { |
||
| 64 | $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); |
||
| 65 | return $this->cardMapper->delete($this->cardMapper->find($id)); |
||
| 66 | } |
||
| 67 | |||
| 68 | public function update($id, $title, $stackId, $type, $order, $description, $owner) { |
||
| 69 | $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); |
||
| 70 | $card = $this->cardMapper->find($id); |
||
| 71 | if ($card->getArchived()) { |
||
| 72 | throw new CardArchivedException(); |
||
| 73 | } |
||
| 74 | $card->setTitle($title); |
||
| 75 | $card->setStackId($stackId); |
||
| 76 | $card->setType($type); |
||
| 77 | $card->setOrder($order); |
||
| 78 | $card->setOwner($owner); |
||
| 79 | $card->setDescription($description); |
||
| 80 | return $this->cardMapper->update($card); |
||
| 81 | } |
||
| 82 | |||
| 83 | View Code Duplication | public function rename($id, $title) { |
|
| 92 | |||
| 93 | public function reorder($id, $stackId, $order) { |
||
| 94 | $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); |
||
| 95 | $cards = $this->cardMapper->findAll($stackId); |
||
| 96 | $result = []; |
||
| 97 | $i = 0; |
||
| 98 | foreach ($cards as $card) { |
||
| 99 | if ($card->getArchived()) { |
||
| 100 | throw new CardArchivedException(); |
||
| 101 | } |
||
| 102 | if ($card->id === $id) { |
||
| 103 | $card->setOrder($order); |
||
| 104 | $card->setLastModified(time()); |
||
| 105 | } |
||
| 106 | |||
| 107 | if ($i === $order) { |
||
| 108 | $i++; |
||
| 109 | } |
||
| 110 | |||
| 111 | if ($card->id !== $id) { |
||
| 112 | $card->setOrder($i++); |
||
| 113 | } |
||
| 114 | $this->cardMapper->update($card); |
||
| 115 | $result[$card->getOrder()] = $card; |
||
| 116 | } |
||
| 117 | |||
| 118 | return $result; |
||
| 119 | } |
||
| 120 | |||
| 121 | View Code Duplication | public function archive($id) { |
|
| 127 | |||
| 128 | View Code Duplication | public function unarchive($id) { |
|
| 134 | |||
| 135 | View Code Duplication | public function assignLabel($cardId, $labelId) { |
|
| 143 | |||
| 144 | View Code Duplication | public function removeLabel($cardId, $labelId) { |
|
| 152 | } |
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: