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 Manager 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 Manager, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 36 | class Manager { |
||
| 37 | |||
| 38 | /** @var IConfig */ |
||
| 39 | protected $config; |
||
| 40 | |||
| 41 | /** @var IDBConnection */ |
||
| 42 | protected $connection; |
||
| 43 | |||
| 44 | /** @var IGroupManager */ |
||
| 45 | protected $groupManager; |
||
| 46 | |||
| 47 | /** @var INotificationManager */ |
||
| 48 | protected $notificationManager; |
||
| 49 | |||
| 50 | /** @var ICommentsManager */ |
||
| 51 | protected $commentsManager; |
||
| 52 | |||
| 53 | /** @var IJobList */ |
||
| 54 | protected $jobList; |
||
| 55 | |||
| 56 | /** @var IUserSession */ |
||
| 57 | protected $userSession; |
||
| 58 | |||
| 59 | /** |
||
| 60 | * @param IConfig $config |
||
| 61 | * @param IDBConnection $connection |
||
| 62 | * @param IGroupManager $groupManager |
||
| 63 | * @param INotificationManager $notificationManager |
||
| 64 | * @param ICommentsManager $commentsManager |
||
| 65 | * @param IJobList $jobList |
||
| 66 | * @param IUserSession $userSession |
||
| 67 | */ |
||
| 68 | 21 | public function __construct(IConfig $config, |
|
| 83 | |||
| 84 | /** |
||
| 85 | * @param string $subject |
||
| 86 | * @param string $message |
||
| 87 | * @param string $user |
||
| 88 | * @param int $time |
||
| 89 | * @param string[] $groups |
||
| 90 | * @param bool $comments |
||
| 91 | * @return array |
||
| 92 | * @throws \InvalidArgumentException when the subject is empty or invalid |
||
| 93 | */ |
||
| 94 | 7 | public function announce($subject, $message, $user, $time, array $groups, $comments) { |
|
| 95 | 7 | $subject = trim($subject); |
|
| 96 | 7 | $message = trim($message); |
|
| 97 | 7 | if (isset($subject[512])) { |
|
| 98 | 1 | throw new \InvalidArgumentException('Invalid subject', 1); |
|
| 99 | } |
||
| 100 | |||
| 101 | 6 | if ($subject === '') { |
|
| 102 | 1 | throw new \InvalidArgumentException('Invalid subject', 2); |
|
| 103 | } |
||
| 104 | |||
| 105 | 5 | $queryBuilder = $this->connection->getQueryBuilder(); |
|
| 106 | 5 | $queryBuilder->insert('announcements') |
|
| 107 | 5 | ->values([ |
|
| 108 | 5 | 'announcement_time' => $queryBuilder->createNamedParameter($time), |
|
| 109 | 5 | 'announcement_user' => $queryBuilder->createNamedParameter($user), |
|
| 110 | 5 | 'announcement_subject' => $queryBuilder->createNamedParameter($subject), |
|
| 111 | 5 | 'announcement_message' => $queryBuilder->createNamedParameter($message), |
|
| 112 | 5 | 'allow_comments' => $queryBuilder->createNamedParameter((int) $comments), |
|
| 113 | ]); |
||
| 114 | 5 | $queryBuilder->execute(); |
|
| 115 | |||
| 116 | 5 | $id = $queryBuilder->getLastInsertId(); |
|
| 117 | |||
| 118 | 5 | $addedGroups = 0; |
|
| 119 | 5 | foreach ($groups as $group) { |
|
| 120 | 5 | if ($this->groupManager->groupExists($group)) { |
|
| 121 | 4 | $this->addGroupLink((int) $id, $group); |
|
| 122 | 5 | $addedGroups++; |
|
| 123 | } |
||
| 124 | } |
||
| 125 | |||
| 126 | 5 | if ($addedGroups === 0) { |
|
| 127 | 4 | $this->addGroupLink((int) $id, 'everyone'); |
|
| 128 | } |
||
| 129 | |||
| 130 | 5 | return $this->getAnnouncement($id, true, true); |
|
| 131 | } |
||
| 132 | |||
| 133 | /** |
||
| 134 | * @param int $announcementId |
||
| 135 | * @param string $group |
||
| 136 | */ |
||
| 137 | 5 | protected function addGroupLink($announcementId, $group) { |
|
| 138 | 5 | $query = $this->connection->getQueryBuilder(); |
|
| 139 | 5 | $query->insert('announcements_groups') |
|
| 140 | 5 | ->values([ |
|
| 141 | 5 | 'announcement_id' => $query->createNamedParameter($announcementId), |
|
| 142 | 5 | 'gid' => $query->createNamedParameter($group), |
|
| 143 | ]); |
||
| 144 | 5 | $query->execute(); |
|
| 145 | 5 | } |
|
| 146 | |||
| 147 | /** |
||
| 148 | * @param int $id |
||
| 149 | */ |
||
| 150 | 5 | public function delete($id) { |
|
| 170 | |||
| 171 | /** |
||
| 172 | * @param int $id |
||
| 173 | * @param bool $parseStrings |
||
| 174 | * @param bool $ignorePermissions |
||
| 175 | * @param bool $returnGroups |
||
| 176 | * @return array |
||
| 177 | * @throws \InvalidArgumentException when the id is invalid |
||
| 178 | */ |
||
| 179 | 6 | public function getAnnouncement($id, $parseStrings = true, $ignorePermissions = false, $returnGroups = true) { |
|
| 180 | 6 | if (!$ignorePermissions) { |
|
| 181 | 4 | $user = $this->userSession->getUser(); |
|
| 182 | 4 | View Code Duplication | if ($user instanceof IUser) { |
| 183 | 2 | $userGroups = $this->groupManager->getUserGroupIds($user); |
|
| 184 | 2 | $userGroups[] = 'everyone'; |
|
| 185 | } else { |
||
| 186 | 2 | $userGroups = ['everyone']; |
|
| 187 | } |
||
| 188 | 4 | $isInAdminGroups = array_intersect($this->getAdminGroups(), $userGroups); |
|
| 189 | |||
| 190 | 4 | if (empty($isInAdminGroups)) { |
|
| 191 | 3 | $query = $this->connection->getQueryBuilder(); |
|
| 192 | 3 | $query->select('*') |
|
| 193 | 3 | ->from('announcements_groups') |
|
| 194 | 3 | ->where($query->expr()->eq('announcement_id', $query->createNamedParameter((int) $id))) |
|
| 195 | 3 | ->andWhere($query->expr()->in('gid', $query->createNamedParameter($userGroups, IQueryBuilder::PARAM_STR_ARRAY))) |
|
| 196 | 3 | ->setMaxResults(1); |
|
| 197 | 3 | $result = $query->execute(); |
|
| 198 | 3 | $entry = $result->fetch(); |
|
| 199 | 3 | $result->closeCursor(); |
|
| 200 | |||
| 201 | 3 | if (!$entry) { |
|
| 202 | 3 | throw new \InvalidArgumentException('Invalid ID'); |
|
| 203 | } |
||
| 204 | } |
||
| 205 | } |
||
| 206 | |||
| 207 | 5 | $queryBuilder = $this->connection->getQueryBuilder(); |
|
| 208 | 5 | $query = $queryBuilder->select('*') |
|
| 209 | 5 | ->from('announcements') |
|
| 210 | 5 | ->where($queryBuilder->expr()->eq('announcement_id', $queryBuilder->createParameter('id'))) |
|
| 211 | 5 | ->setParameter('id', (int) $id); |
|
| 212 | 5 | $result = $query->execute(); |
|
| 213 | 5 | $row = $result->fetch(); |
|
| 214 | 5 | $result->closeCursor(); |
|
| 215 | |||
| 216 | 5 | if ($row === false) { |
|
| 217 | 3 | throw new \InvalidArgumentException('Invalid ID'); |
|
| 218 | } |
||
| 219 | |||
| 220 | 5 | $groups = null; |
|
| 221 | 5 | if ($returnGroups && ($ignorePermissions || !empty($isInAdminGroups))) { |
|
| 222 | 5 | $groups = $this->getGroups((int) $id); |
|
| 223 | } |
||
| 224 | |||
| 225 | $announcement = [ |
||
| 226 | 5 | 'id' => (int) $row['announcement_id'], |
|
| 227 | 5 | 'author' => $row['announcement_user'], |
|
| 228 | 5 | 'time' => (int) $row['announcement_time'], |
|
| 229 | 5 | 'subject' => $parseStrings ? $this->parseSubject($row['announcement_subject']) : $row['announcement_subject'], |
|
| 230 | 5 | 'message' => $parseStrings ? $this->parseMessage($row['announcement_message']) : $row['announcement_message'], |
|
| 231 | 5 | 'groups' => $groups, |
|
| 232 | 5 | 'comments' => $row['allow_comments'] ? 0 : false, |
|
| 233 | ]; |
||
| 234 | |||
| 235 | 5 | if ($ignorePermissions || !empty($isInAdminGroups)) { |
|
| 236 | 5 | $announcement['notifications'] = $this->hasNotifications((int) $id); |
|
| 237 | } |
||
| 238 | |||
| 239 | 5 | return $announcement; |
|
| 240 | } |
||
| 241 | |||
| 242 | /** |
||
| 243 | * @param int $limit |
||
| 244 | * @param int $offset |
||
| 245 | * @param bool $parseStrings |
||
| 246 | * @return array |
||
| 247 | */ |
||
| 248 | 3 | public function getAnnouncements($limit = 15, $offset = 0, $parseStrings = true) { |
|
| 249 | 3 | $query = $this->connection->getQueryBuilder(); |
|
| 250 | 3 | $query->select('a.*') |
|
| 251 | 3 | ->from('announcements', 'a') |
|
| 252 | 3 | ->orderBy('a.announcement_time', 'DESC') |
|
| 253 | 3 | ->groupBy('a.announcement_id') |
|
| 254 | 3 | ->setMaxResults($limit); |
|
| 255 | |||
| 256 | 3 | $user = $this->userSession->getUser(); |
|
| 257 | 3 | View Code Duplication | if ($user instanceof IUser) { |
| 258 | 2 | $userGroups = $this->groupManager->getUserGroupIds($user); |
|
| 259 | 2 | $userGroups[] = 'everyone'; |
|
| 260 | } else { |
||
| 261 | 1 | $userGroups = ['everyone']; |
|
| 262 | } |
||
| 263 | |||
| 264 | 3 | $isInAdminGroups = array_intersect($this->getAdminGroups(), $userGroups); |
|
| 265 | 3 | if (empty($isInAdminGroups)) { |
|
| 266 | 2 | $query->leftJoin('a', 'announcements_groups', 'ag', $query->expr()->eq( |
|
| 267 | 2 | 'a.announcement_id', 'ag.announcement_id' |
|
| 268 | )) |
||
| 269 | 2 | ->andWhere($query->expr()->in('ag.gid', $query->createNamedParameter($userGroups, IQueryBuilder::PARAM_STR_ARRAY))); |
|
| 270 | } |
||
| 271 | |||
| 272 | 3 | if ($offset > 0) { |
|
| 273 | 3 | $query->andWhere($query->expr()->lt('a.announcement_id', $query->createNamedParameter($offset, IQueryBuilder::PARAM_INT))); |
|
| 274 | } |
||
| 275 | |||
| 276 | 3 | $result = $query->execute(); |
|
| 277 | |||
| 278 | 3 | $announcements = []; |
|
| 279 | 3 | while ($row = $result->fetch()) { |
|
| 280 | 3 | $id = (int) $row['announcement_id']; |
|
| 281 | 3 | $announcements[$id] = [ |
|
| 282 | 3 | 'id' => $id, |
|
| 283 | 3 | 'author' => $row['announcement_user'], |
|
| 284 | 3 | 'time' => (int) $row['announcement_time'], |
|
| 285 | 3 | 'subject' => $parseStrings ? $this->parseSubject($row['announcement_subject']) : $row['announcement_subject'], |
|
| 286 | 3 | 'message' => $parseStrings ? $this->parseMessage($row['announcement_message']) : $row['announcement_message'], |
|
| 287 | 'groups' => null, |
||
| 288 | 3 | 'comments' => $row['allow_comments'] ? $this->getNumberOfComments($id) : false, |
|
| 289 | ]; |
||
| 290 | |||
| 291 | 3 | if (!empty($isInAdminGroups)) { |
|
| 292 | 1 | $announcements[$id]['notifications'] = $this->hasNotifications($id); |
|
| 293 | } |
||
| 294 | } |
||
| 295 | 3 | $result->closeCursor(); |
|
| 296 | |||
| 297 | 3 | if (!empty($isInAdminGroups)) { |
|
| 298 | 1 | $allGroups = $this->getGroups(array_keys($announcements)); |
|
| 299 | 1 | foreach ($allGroups as $id => $groups) { |
|
| 300 | 1 | $announcements[$id]['groups'] = $groups; |
|
| 301 | } |
||
| 302 | } |
||
| 303 | |||
| 304 | 3 | return $announcements; |
|
| 305 | } |
||
| 306 | |||
| 307 | /** |
||
| 308 | * Return the groups (or string everyone) which have access to the announcement(s) |
||
| 309 | * |
||
| 310 | * @param int|int[] $ids |
||
| 311 | * @return string[]|array[] |
||
| 312 | */ |
||
| 313 | 5 | public function getGroups($ids) { |
|
| 314 | 5 | $returnSingleResult = false; |
|
| 315 | 5 | if (is_int($ids)) { |
|
| 316 | 5 | $ids = [$ids]; |
|
| 317 | 5 | $returnSingleResult = true; |
|
| 318 | } |
||
| 319 | |||
| 320 | 5 | $query = $this->connection->getQueryBuilder(); |
|
| 321 | 5 | $query->select('*') |
|
| 322 | 5 | ->from('announcements_groups') |
|
| 323 | 5 | ->where($query->expr()->in('announcement_id', $query->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); |
|
| 324 | 5 | $result = $query->execute(); |
|
| 325 | |||
| 326 | 5 | $groups = []; |
|
| 327 | 5 | while ($row = $result->fetch()) { |
|
| 328 | 5 | if (!isset($groups[(int) $row['announcement_id']])) { |
|
| 329 | 5 | $groups[(int) $row['announcement_id']] = []; |
|
| 330 | } |
||
| 331 | 5 | $groups[(int) $row['announcement_id']][] = $row['gid']; |
|
| 332 | } |
||
| 333 | 5 | $result->closeCursor(); |
|
| 334 | |||
| 335 | 5 | return $returnSingleResult ? (array) array_pop($groups) : $groups; |
|
| 336 | } |
||
| 337 | |||
| 338 | /** |
||
| 339 | * @param int $id |
||
| 340 | */ |
||
| 341 | public function markNotificationRead($id) { |
||
| 342 | $user = $this->userSession->getUser(); |
||
| 343 | |||
| 344 | if ($user instanceof IUser) { |
||
| 345 | $notification = $this->notificationManager->createNotification(); |
||
| 346 | $notification->setApp('announcementcenter') |
||
| 347 | ->setUser($user->getUID()) |
||
| 348 | ->setObject('announcement', $id); |
||
| 349 | $this->notificationManager->markProcessed($notification); |
||
| 350 | } |
||
| 351 | } |
||
| 352 | |||
| 353 | /** |
||
| 354 | * @param int $id |
||
| 355 | * @return int |
||
| 356 | */ |
||
| 357 | 2 | protected function getNumberOfComments($id) { |
|
| 360 | |||
| 361 | /** |
||
| 362 | * @param int $id |
||
| 363 | * @return bool |
||
| 364 | */ |
||
| 365 | 5 | protected function hasNotifications($id) { |
|
| 366 | 5 | $hasJob = $this->jobList->has('OCA\AnnouncementCenter\BackgroundJob', [ |
|
| 367 | 5 | 'id' => $id, |
|
| 368 | 'activities' => true, |
||
| 369 | 'notifications' => true, |
||
| 370 | ]); |
||
| 371 | |||
| 372 | 5 | $hasJob = $hasJob || $this->jobList->has('OCA\AnnouncementCenter\BackgroundJob', [ |
|
| 373 | 5 | 'id' => $id, |
|
| 374 | 'activities' => false, |
||
| 375 | 'notifications' => true, |
||
| 376 | ]); |
||
| 377 | |||
| 378 | 5 | if ($hasJob) { |
|
| 379 | return true; |
||
| 380 | } |
||
| 381 | |||
| 382 | 5 | $notification = $this->notificationManager->createNotification(); |
|
| 383 | 5 | $notification->setApp('announcementcenter') |
|
| 384 | 5 | ->setObject('announcement', $id); |
|
| 385 | 5 | return $this->notificationManager->getCount($notification) > 0; |
|
| 386 | } |
||
| 387 | |||
| 388 | /** |
||
| 389 | * @param int $id |
||
| 390 | */ |
||
| 391 | public function removeNotifications($id) { |
||
| 392 | if ($this->jobList->has('OCA\AnnouncementCenter\BackgroundJob', [ |
||
| 393 | 'id' => $id, |
||
| 394 | 'activities' => true, |
||
| 395 | 'notifications' => true, |
||
| 396 | ])) { |
||
| 397 | // Delete the current background job and add a new one without notifications |
||
| 398 | $this->jobList->remove('OCA\AnnouncementCenter\BackgroundJob', [ |
||
| 399 | 'id' => $id, |
||
| 400 | 'activities' => true, |
||
| 401 | 'notifications' => true, |
||
| 402 | ]); |
||
| 403 | $this->jobList->add('OCA\AnnouncementCenter\BackgroundJob', [ |
||
| 404 | 'id' => $id, |
||
| 405 | 'activities' => true, |
||
| 406 | 'notifications' => false, |
||
| 407 | ]); |
||
| 408 | |||
| 409 | } else { |
||
| 410 | $this->jobList->remove('OCA\AnnouncementCenter\BackgroundJob', [ |
||
| 411 | 'id' => $id, |
||
| 412 | 'activities' => false, |
||
| 413 | 'notifications' => true, |
||
| 414 | ]); |
||
| 415 | } |
||
| 416 | |||
| 417 | $notification = $this->notificationManager->createNotification(); |
||
| 418 | $notification->setApp('announcementcenter') |
||
| 419 | ->setObject('announcement', $id); |
||
| 420 | $this->notificationManager->markProcessed($notification); |
||
| 421 | } |
||
| 422 | |||
| 423 | /** |
||
| 424 | * @param string $message |
||
| 425 | * @return string |
||
| 426 | */ |
||
| 427 | 5 | protected function parseMessage($message) { |
|
| 430 | |||
| 431 | /** |
||
| 432 | * @param string $subject |
||
| 433 | * @return string |
||
| 434 | */ |
||
| 435 | 5 | protected function parseSubject($subject) { |
|
| 438 | |||
| 439 | /** |
||
| 440 | * Check if the user is in the admin group |
||
| 441 | * @return bool |
||
| 442 | */ |
||
| 443 | 6 | public function checkIsAdmin() { |
|
| 444 | 6 | $user = $this->userSession->getUser(); |
|
| 445 | |||
| 446 | 6 | if ($user instanceof IUser) { |
|
| 447 | 5 | $groups = $this->getAdminGroups(); |
|
| 448 | 5 | foreach ($groups as $group) { |
|
| 449 | 5 | if ($this->groupManager->isInGroup($user->getUID(), $group)) { |
|
| 450 | 5 | return true; |
|
| 451 | } |
||
| 452 | } |
||
| 453 | } |
||
| 454 | |||
| 455 | 3 | return false; |
|
| 456 | } |
||
| 457 | |||
| 458 | 9 | protected function getAdminGroups() { |
|
| 463 | } |
||
| 464 |
This error could be the result of:
1. Missing dependencies
PHP Analyzer uses your
composer.jsonfile (if available) to determine the dependencies of your project and to determine all the available classes and functions. It expects thecomposer.jsonto be in the root folder of your repository.Are you sure this class is defined by one of your dependencies, or did you maybe not list a dependency in either the
requireorrequire-devsection?2. Missing use statement
PHP does not complain about undefined classes in
ìnstanceofchecks. For example, the following PHP code will work perfectly fine:If you have not tested against this specific condition, such errors might go unnoticed.