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 RepairUnmergedShares 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 RepairUnmergedShares, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 44 | class RepairUnmergedShares implements IRepairStep { |
||
| 45 | |||
| 46 | /** @var \OCP\IConfig */ |
||
| 47 | protected $config; |
||
| 48 | |||
| 49 | /** @var \OCP\IDBConnection */ |
||
| 50 | protected $connection; |
||
| 51 | |||
| 52 | /** @var IUserManager */ |
||
| 53 | protected $userManager; |
||
| 54 | |||
| 55 | /** @var IGroupManager */ |
||
| 56 | protected $groupManager; |
||
| 57 | |||
| 58 | /** @var IQueryBuilder */ |
||
| 59 | private $queryGetSharesWithUsers; |
||
| 60 | |||
| 61 | /** @var IQueryBuilder */ |
||
| 62 | private $queryUpdateSharePermissionsAndTarget; |
||
| 63 | |||
| 64 | /** @var IQueryBuilder */ |
||
| 65 | private $queryUpdateShareInBatch; |
||
| 66 | |||
| 67 | /** |
||
| 68 | * @param \OCP\IConfig $config |
||
| 69 | * @param \OCP\IDBConnection $connection |
||
| 70 | */ |
||
| 71 | public function __construct( |
||
| 72 | IConfig $config, |
||
| 73 | IDBConnection $connection, |
||
| 74 | IUserManager $userManager, |
||
| 75 | IGroupManager $groupManager |
||
| 76 | ) { |
||
| 77 | $this->connection = $connection; |
||
| 78 | $this->config = $config; |
||
| 79 | $this->userManager = $userManager; |
||
| 80 | $this->groupManager = $groupManager; |
||
| 81 | } |
||
| 82 | |||
| 83 | public function getName() { |
||
| 86 | |||
| 87 | /** |
||
| 88 | * Builds prepared queries for reuse |
||
| 89 | */ |
||
| 90 | private function buildPreparedQueries() { |
||
| 130 | |||
| 131 | private function getSharesWithUser($shareType, $shareWiths) { |
||
| 150 | |||
| 151 | private function isPotentialDuplicateName($name) { |
||
| 154 | |||
| 155 | /** |
||
| 156 | * Decide on the best target name based on all group shares and subshares, |
||
| 157 | * goal is to increase the likeliness that the chosen name matches what |
||
| 158 | * the user is expecting. |
||
| 159 | * |
||
| 160 | * For this, we discard the entries with parenthesis "(2)". |
||
| 161 | * In case the user also renamed the duplicates to a legitimate name, this logic |
||
| 162 | * will still pick the most recent one as it's the one the user is most likely to |
||
| 163 | * remember renaming. |
||
| 164 | * |
||
| 165 | * If no suitable subshare is found, use the least recent group share instead. |
||
| 166 | * |
||
| 167 | * @param array $groupShares group share entries |
||
| 168 | * @param array $subShares sub share entries |
||
| 169 | * |
||
| 170 | * @return string chosen target name |
||
| 171 | */ |
||
| 172 | private function findBestTargetName($groupShares, $subShares) { |
||
| 196 | |||
| 197 | /** |
||
| 198 | * Fix the given received share represented by the set of group shares |
||
| 199 | * and matching sub shares |
||
| 200 | * |
||
| 201 | * @param array $groupShares group share entries |
||
| 202 | * @param array $subShares sub share entries |
||
| 203 | * |
||
| 204 | * @return boolean false if the share was not repaired, true if it was |
||
| 205 | */ |
||
| 206 | private function fixThisShare($groupShares, $subShares) { |
||
| 207 | if (empty($subShares)) { |
||
| 208 | return false; |
||
| 209 | } |
||
| 210 | |||
| 211 | $groupSharesById = []; |
||
| 212 | foreach ($groupShares as $groupShare) { |
||
| 213 | $groupSharesById[$groupShare['id']] = $groupShare; |
||
| 214 | } |
||
| 215 | |||
| 216 | if ($this->isThisShareValid($groupSharesById, $subShares)) { |
||
| 217 | return false; |
||
| 218 | } |
||
| 219 | |||
| 220 | $targetPath = $this->findBestTargetName($groupShares, $subShares); |
||
| 221 | |||
| 222 | // check whether the user opted out completely of all subshares |
||
| 223 | $optedOut = true; |
||
| 224 | foreach ($subShares as $subShare) { |
||
| 225 | if ((int)$subShare['permissions'] !== 0) { |
||
| 226 | $optedOut = false; |
||
| 227 | break; |
||
| 228 | } |
||
| 229 | } |
||
| 230 | |||
| 231 | $shareIds = []; |
||
| 232 | foreach ($subShares as $subShare) { |
||
| 233 | // only if the user deleted some subshares but not all, adjust the permissions of that subshare |
||
| 234 | if (!$optedOut && (int)$subShare['permissions'] === 0 && (int)$subShare['share_type'] === DefaultShareProvider::SHARE_TYPE_USERGROUP) { |
||
| 235 | // set permissions from parent group share |
||
| 236 | $permissions = $groupSharesById[$subShare['parent']]['permissions']; |
||
| 237 | |||
| 238 | // fix permissions and target directly |
||
| 239 | $query = $this->queryUpdateSharePermissionsAndTarget; |
||
| 240 | $query->setParameter('shareid', $subShare['id']); |
||
| 241 | $query->setParameter('file_target', $targetPath); |
||
| 242 | $query->setParameter('permissions', $permissions); |
||
| 243 | $query->execute(); |
||
| 244 | } else { |
||
| 245 | // gather share ids for bulk target update |
||
| 246 | if ($subShare['file_target'] !== $targetPath) { |
||
| 247 | $shareIds[] = (int)$subShare['id']; |
||
| 248 | } |
||
| 249 | } |
||
| 250 | } |
||
| 251 | |||
| 252 | if (!empty($shareIds)) { |
||
| 253 | $query = $this->queryUpdateShareInBatch; |
||
| 254 | $query->setParameter('ids', $shareIds, IQueryBuilder::PARAM_INT_ARRAY); |
||
| 255 | $query->setParameter('file_target', $targetPath); |
||
| 256 | $query->execute(); |
||
| 257 | } |
||
| 258 | |||
| 259 | return true; |
||
| 260 | } |
||
| 261 | |||
| 262 | /** |
||
| 263 | * Checks whether the number of group shares is balanced with the child subshares. |
||
| 264 | * If all group shares have exactly one subshare, and the target of every subshare |
||
| 265 | * is the same, then the share is valid. |
||
| 266 | * If however there is a group share entry that has no matching subshare, it means |
||
| 267 | * we're in the bogus situation and the whole share must be repaired |
||
| 268 | * |
||
| 269 | * @param array $groupSharesById |
||
| 270 | * @param array $subShares |
||
| 271 | * |
||
| 272 | * @return true if the share is valid, false if it needs repair |
||
| 273 | */ |
||
| 274 | private function isThisShareValid($groupSharesById, $subShares) { |
||
| 295 | |||
| 296 | /** |
||
| 297 | * Detect unmerged received shares and merge them properly |
||
| 298 | */ |
||
| 299 | private function fixUnmergedShares(IOutput $out, IUser $user) { |
||
| 300 | $groups = $this->groupManager->getUserGroupIds($user); |
||
| 301 | if (empty($groups)) { |
||
| 302 | // user is in no groups, so can't have received group shares |
||
| 303 | return; |
||
| 304 | } |
||
| 305 | |||
| 306 | // get all subshares grouped by item source |
||
| 307 | $subSharesByItemSource = $this->getSharesWithUser(DefaultShareProvider::SHARE_TYPE_USERGROUP, [$user->getUID()]); |
||
| 308 | |||
| 309 | // because sometimes one wants to give the user more permissions than the group share |
||
| 310 | $userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]); |
||
| 311 | |||
| 312 | if (empty($subSharesByItemSource) && empty($userSharesByItemSource)) { |
||
| 313 | // nothing to repair for this user, no need to do extra queries |
||
| 314 | return; |
||
| 315 | } |
||
| 316 | |||
| 317 | $groupSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_GROUP, $groups); |
||
| 318 | if (empty($groupSharesByItemSource) && empty($userSharesByItemSource)) { |
||
| 319 | // nothing to repair for this user |
||
| 320 | return; |
||
| 321 | } |
||
| 322 | |||
| 323 | foreach ($groupSharesByItemSource as $itemSource => $groupShares) { |
||
| 324 | $subShares = []; |
||
| 325 | if (isset($subSharesByItemSource[$itemSource])) { |
||
| 326 | $subShares = $subSharesByItemSource[$itemSource]; |
||
| 327 | } |
||
| 328 | |||
| 329 | if (isset($userSharesByItemSource[$itemSource])) { |
||
| 330 | // add it to the subshares to get a similar treatment |
||
| 331 | $subShares = array_merge($subShares, $userSharesByItemSource[$itemSource]); |
||
| 332 | } |
||
| 333 | |||
| 334 | $this->fixThisShare($groupShares, $subShares); |
||
| 335 | } |
||
| 336 | } |
||
| 337 | |||
| 338 | /** |
||
| 339 | * Count all the users |
||
| 340 | * |
||
| 341 | * @return int |
||
| 342 | */ |
||
| 343 | View Code Duplication | private function countUsers() { |
|
| 353 | |||
| 354 | public function run(IOutput $output) { |
||
| 374 | } |
||
| 375 |
This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue.
If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress.
In this case you can add the
@ignorePhpDoc annotation to the duplicate definition and it will be ignored.