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 AbstractSql 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 AbstractSql, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
35 | abstract class AbstractSql |
||
36 | { |
||
37 | const TICKETS_TABLE = 'tickets'; |
||
38 | const PERFORMERS_TABLE = 'performers'; |
||
39 | const TICKETS_X_PERFORMERS_TABLE = 'tickets_x_performers'; |
||
40 | const CODE_LENGTH = 6; |
||
41 | |||
42 | /** |
||
43 | * @var Connection |
||
44 | */ |
||
45 | protected $dbConn; |
||
46 | /** |
||
47 | * @var int |
||
48 | */ |
||
49 | protected $upcomingCount = 3; |
||
50 | |||
51 | /** |
||
52 | * @var LoggerInterface |
||
53 | */ |
||
54 | protected $logger; |
||
55 | |||
56 | /** |
||
57 | * DataSource constructor. |
||
58 | * |
||
59 | * @param $dbConn |
||
60 | */ |
||
61 | public function __construct(Connection $dbConn) |
||
65 | |||
66 | /** |
||
67 | * @return Connection |
||
68 | */ |
||
69 | public function getDbConn() |
||
73 | |||
74 | /** |
||
75 | * @return int |
||
76 | */ |
||
77 | public function getUpcomingCount() |
||
81 | |||
82 | /** |
||
83 | * @param int $upcomingCount |
||
84 | */ |
||
85 | public function setUpcomingCount($upcomingCount) |
||
89 | |||
90 | /** |
||
91 | * @return LoggerInterface |
||
92 | */ |
||
93 | public function getLogger() |
||
101 | |||
102 | /** |
||
103 | * @param LoggerInterface $logger |
||
104 | */ |
||
105 | public function setLogger(LoggerInterface $logger) |
||
109 | |||
110 | /** |
||
111 | * @param $songId |
||
112 | * |
||
113 | * @return array |
||
114 | */ |
||
115 | public function fetchSongRowById($songId) |
||
124 | |||
125 | /** |
||
126 | * Fill out names of linked object ids for given ticket |
||
127 | * |
||
128 | * @param $ticket |
||
129 | * |
||
130 | * @return mixed |
||
131 | */ |
||
132 | public function expandTicketData($ticket) |
||
147 | |||
148 | /** |
||
149 | * expandTicketData for multiple tickets |
||
150 | * |
||
151 | * @param $tickets |
||
152 | * |
||
153 | * @return mixed |
||
154 | */ |
||
155 | public function expandTicketsData($tickets) |
||
163 | |||
164 | |||
165 | /** |
||
166 | * Fill out names of linked object ids for given song |
||
167 | * |
||
168 | * @param $song |
||
169 | * @return mixed |
||
170 | */ |
||
171 | public function expandSongData($song) |
||
201 | |||
202 | public function isPotentialCodeNumber($searchString) |
||
209 | |||
210 | /** |
||
211 | * Get performed, pending data for all performers |
||
212 | * |
||
213 | * @return array |
||
214 | */ |
||
215 | public function generatePerformerStats() |
||
229 | |||
230 | /** |
||
231 | * Save band to ticket |
||
232 | * |
||
233 | * @param $ticketId |
||
234 | * @param array $band ['instrumentCode' => 'name'] FIXME update |
||
235 | */ |
||
236 | public function storeBandToTicket($ticketId, $band) |
||
267 | |||
268 | public function fetchPerformerIdByName($performerName, $createMissing = false) |
||
283 | |||
284 | /** |
||
285 | * Fetch all performers on a song with their stats |
||
286 | * |
||
287 | * @deprecated Use fetchPerformersWithInstrumentByTicketId() |
||
288 | * |
||
289 | * @param $ticketId |
||
290 | * |
||
291 | * @return array[] |
||
292 | */ |
||
293 | public function fetchPerformersByTicketId($ticketId) |
||
312 | |||
313 | public function fetchPerformersWithInstrumentByTicketId($ticketId) |
||
343 | |||
344 | /** |
||
345 | * @param $searchString |
||
346 | * @param $howMany |
||
347 | * |
||
348 | * @return array |
||
349 | */ |
||
350 | public function findSongsBySearchString($searchString, $howMany = 10) |
||
399 | |||
400 | /** |
||
401 | * @param $id |
||
402 | * |
||
403 | * @return int |
||
404 | */ |
||
405 | public function markTicketUsedById($id) |
||
410 | |||
411 | /** |
||
412 | * @param $id |
||
413 | * |
||
414 | * @return int |
||
415 | */ |
||
416 | public function deleteTicketById($id) |
||
421 | |||
422 | /** |
||
423 | * @param $id |
||
424 | * @param $offset |
||
425 | * |
||
426 | * @return mixed |
||
427 | */ |
||
428 | public function updateTicketOffsetById($id, $offset) |
||
448 | |||
449 | /** |
||
450 | * @param $songKey |
||
451 | * |
||
452 | * @return array |
||
453 | */ |
||
454 | public function fetchSongByKey($songKey) |
||
465 | |||
466 | /** |
||
467 | * @param bool $includePrivate |
||
468 | * |
||
469 | * @return array|mixed |
||
470 | * |
||
471 | * @throws \Doctrine\DBAL\DBALException |
||
472 | */ |
||
473 | public function fetchUpcomingTickets($includePrivate = false) |
||
488 | |||
489 | /** |
||
490 | * Fetch all non-deleted tickets in offset order |
||
491 | * |
||
492 | * @return array|mixed |
||
493 | * |
||
494 | * @throws \Doctrine\DBAL\DBALException |
||
495 | */ |
||
496 | View Code Duplication | public function fetchUndeletedTickets() |
|
506 | |||
507 | /** |
||
508 | * Fetch all performed tickets in offset order |
||
509 | * |
||
510 | * @return array |
||
511 | * |
||
512 | * @throws \Doctrine\DBAL\DBALException |
||
513 | */ |
||
514 | View Code Duplication | public function fetchPerformedTickets() |
|
524 | |||
525 | /** |
||
526 | * Check whether song is already in the queue |
||
527 | * |
||
528 | * @param $songId |
||
529 | * @return array|mixed |
||
530 | * @throws \Doctrine\DBAL\DBALException |
||
531 | */ |
||
532 | View Code Duplication | public function getQueueEntriesForSongId($songId) |
|
544 | |||
545 | /** |
||
546 | * @param $title |
||
547 | * @param $songId |
||
548 | * |
||
549 | * @param null $userId |
||
550 | * @return false|int Row ID |
||
551 | */ |
||
552 | public function storeNewTicket($title, $songId, $userId = null) |
||
573 | |||
574 | /** |
||
575 | * Normalise datatypes returned in song query |
||
576 | * |
||
577 | * @param $song |
||
578 | * |
||
579 | * @return mixed |
||
580 | */ |
||
581 | public function normaliseSongRecord($song) |
||
601 | |||
602 | /** |
||
603 | * @param $id |
||
604 | * @param $fields |
||
605 | * |
||
606 | * @return int Number of updated rows |
||
607 | */ |
||
608 | public function updateTicketById($id, $fields) |
||
616 | |||
617 | /** |
||
618 | * Fetch core ticket data (not band, etc) by ticket id |
||
619 | * |
||
620 | * @param $id |
||
621 | * |
||
622 | * @return array |
||
623 | */ |
||
624 | View Code Duplication | public function fetchTicketById($id) |
|
631 | |||
632 | /** |
||
633 | * Get current value of a named setting, NULL if missing |
||
634 | * |
||
635 | * @param $key |
||
636 | * @return mixed|null |
||
637 | */ |
||
638 | public function fetchSetting($key) |
||
645 | |||
646 | |||
647 | public function settingExists($key) |
||
654 | |||
655 | public function updateSetting($k, $v) |
||
665 | |||
666 | /** |
||
667 | * Return SQL in appropriate dialect to concatenate the listed values |
||
668 | * |
||
669 | * @param array $fields |
||
670 | * |
||
671 | * @return string |
||
672 | */ |
||
673 | abstract protected function concatenateEscapedFields($fields); |
||
674 | |||
675 | /** |
||
676 | * Return ticket array with fields converted to correct datatype |
||
677 | * |
||
678 | * @param $ticket |
||
679 | * |
||
680 | * @return mixed |
||
681 | */ |
||
682 | protected function normaliseTicketRecord($ticket) |
||
697 | |||
698 | /** |
||
699 | * Delete all song & performer data |
||
700 | * |
||
701 | * @throws \Doctrine\DBAL\DBALException |
||
702 | */ |
||
703 | public function resetAllSessionData() |
||
719 | |||
720 | /** |
||
721 | * Delete all catalogue data |
||
722 | * |
||
723 | * @throws \Doctrine\DBAL\DBALException |
||
724 | */ |
||
725 | public function resetCatalogue() |
||
746 | |||
747 | /** |
||
748 | * Store an instrument to DB |
||
749 | * |
||
750 | * @param Instrument $instrument |
||
751 | */ |
||
752 | public function storeInstrument(Instrument $instrument) |
||
759 | |||
760 | /** |
||
761 | * Store a platform to DB |
||
762 | * |
||
763 | * @param Platform $platform |
||
764 | */ |
||
765 | public function storePlatform(Platform $platform) |
||
772 | |||
773 | /** |
||
774 | * Store a source to DB |
||
775 | * |
||
776 | * @param Source $source |
||
777 | */ |
||
778 | public function storeSource(Source $source) |
||
785 | |||
786 | /** |
||
787 | * Store a song to DB |
||
788 | * |
||
789 | * @param Song $song |
||
790 | */ |
||
791 | public function storeSong(Song $song) |
||
800 | |||
801 | /** |
||
802 | * @param string $sourceName |
||
803 | * |
||
804 | * @return null|Source |
||
805 | */ |
||
806 | View Code Duplication | public function fetchSourceByName($sourceName) |
|
823 | |||
824 | |||
825 | /** |
||
826 | * @param $sourceId |
||
827 | * @return null|Source |
||
828 | */ |
||
829 | View Code Duplication | public function fetchSourceById($sourceId) |
|
846 | |||
847 | View Code Duplication | public function fetchPlatformByName($platformName) |
|
864 | |||
865 | /** |
||
866 | * Fetch all available platforms |
||
867 | * |
||
868 | * @return Platform[] |
||
869 | */ |
||
870 | public function fetchAllPlatforms() |
||
886 | |||
887 | /** |
||
888 | * Fetch all available instruments |
||
889 | * |
||
890 | * @return Instrument[] |
||
891 | */ |
||
892 | public function fetchAllInstruments() |
||
908 | |||
909 | /** |
||
910 | * @param $songId |
||
911 | * @param array $platformIds |
||
912 | */ |
||
913 | View Code Duplication | public function storeSongPlatformLinks($songId, array $platformIds) |
|
921 | |||
922 | /** |
||
923 | * @param $name |
||
924 | * @return null|Instrument |
||
925 | */ |
||
926 | View Code Duplication | public function fetchInstrumentByName($name) |
|
943 | |||
944 | View Code Duplication | protected function fetchInstrumentByAbbreviation($abbreviation) |
|
961 | |||
962 | |||
963 | /** |
||
964 | * @param $songId |
||
965 | * @param array $instrumentIds |
||
966 | * @throws \Doctrine\DBAL\Exception\InvalidArgumentException |
||
967 | */ |
||
968 | View Code Duplication | public function storeSongInstrumentLinks($songId, array $instrumentIds) |
|
976 | |||
977 | /** |
||
978 | * @param $songId |
||
979 | * @return Instrument[] |
||
980 | */ |
||
981 | View Code Duplication | public function fetchInstrumentsForSongId($songId) |
|
999 | |||
1000 | /** |
||
1001 | * @param $songId |
||
1002 | * @return Platform[] |
||
1003 | */ |
||
1004 | View Code Duplication | public function fetchPlatformsForSongId($songId) |
|
1022 | |||
1023 | /** |
||
1024 | * @param $row |
||
1025 | * @return Instrument |
||
1026 | */ |
||
1027 | protected function buildInstrumentFromDbRow($row) |
||
1037 | |||
1038 | /** |
||
1039 | * @param $row |
||
1040 | * @return Platform |
||
1041 | */ |
||
1042 | protected function buildPlatformFromDbRow($row) |
||
1050 | |||
1051 | /** |
||
1052 | * @param $row |
||
1053 | * @return Source |
||
1054 | */ |
||
1055 | protected function buildSourceFromDbRow($row) |
||
1063 | |||
1064 | /** |
||
1065 | * @param Song $song |
||
1066 | * @return array |
||
1067 | */ |
||
1068 | protected function songToDbRow(Song $song) |
||
1082 | |||
1083 | /** |
||
1084 | * @param Source $source |
||
1085 | * @return array |
||
1086 | */ |
||
1087 | protected function sourceToDbRow(Source $source) |
||
1097 | |||
1098 | /** |
||
1099 | * @param Instrument $instrument |
||
1100 | * @return array |
||
1101 | */ |
||
1102 | protected function instrumentToDbRow(Instrument $instrument) |
||
1114 | |||
1115 | /** |
||
1116 | * @param Platform $platform |
||
1117 | * @return array |
||
1118 | */ |
||
1119 | protected function platformToDbRow(Platform $platform) |
||
1129 | } |
||
1130 |
This check marks implicit conversions of arrays to boolean values in a comparison. While in PHP an empty array is considered to be equal (but not identical) to false, this is not always apparent.
Consider making the comparison explicit by using
empty(..)
or! empty(...)
instead.