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 NewbbOnlineHandler 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 NewbbOnlineHandler, and based on these observations, apply Extract Interface, too.
1 | <?php |
||
21 | class NewbbOnlineHandler |
||
22 | { |
||
23 | public $db; |
||
24 | public $forum_id; |
||
25 | public $forumObject; |
||
26 | public $topic_id; |
||
27 | public $user_ids = []; |
||
28 | |||
29 | /** |
||
30 | * @param null|\NewbbForum $forum |
||
31 | * @param null|Topic $forumtopic |
||
32 | */ |
||
33 | public function init($forum = null, $forumtopic = null) |
||
55 | |||
56 | public function update() |
||
84 | |||
85 | /** |
||
86 | * @param $xoopsTpl |
||
87 | */ |
||
88 | public function render(Smarty $xoopsTpl) |
||
143 | |||
144 | /** |
||
145 | * Deprecated |
||
146 | */ |
||
147 | public function showOnline() |
||
148 | { |
||
149 | include_once __DIR__ . '/../include/functions.render.php'; |
||
150 | include_once __DIR__ . '/../include/functions.user.php'; |
||
151 | $criteria = null; |
||
152 | View Code Duplication | if ($this->topic_id) { |
|
153 | $criteria = new Criteria('online_topic', $this->topic_id); |
||
154 | } elseif ($this->forum_id) { |
||
155 | $criteria = new Criteria('online_forum', $this->forum_id); |
||
156 | } |
||
157 | $users = $this->getAll($criteria); |
||
158 | $num_total = count($users); |
||
159 | |||
160 | $num_user = 0; |
||
161 | $users_id = []; |
||
162 | $users_online = []; |
||
163 | View Code Duplication | for ($i = 0; $i < $num_total; ++$i) { |
|
164 | if (empty($users[$i]['online_uid'])) { |
||
165 | continue; |
||
166 | } |
||
167 | $users_id[] = $users[$i]['online_uid']; |
||
168 | $users_online[$users[$i]['online_uid']] = [ |
||
169 | 'link' => XOOPS_URL . '/userinfo.php?uid=' . $users[$i]['online_uid'], |
||
170 | 'uname' => $users[$i]['online_uname'] |
||
171 | ]; |
||
172 | ++$num_user; |
||
173 | } |
||
174 | $num_anonymous = $num_total - $num_user; |
||
175 | $online = []; |
||
176 | $online['image'] = newbbDisplayImage('whosonline'); |
||
177 | $online['statistik'] = newbbDisplayImage('statistik'); |
||
178 | $online['num_total'] = $num_total; |
||
179 | $online['num_user'] = $num_user; |
||
180 | $online['num_anonymous'] = $num_anonymous; |
||
181 | $administrator_list = newbbIsModuleAdministrators($users_id); |
||
182 | $moderator_list = []; |
||
183 | View Code Duplication | if ($member_list = array_diff($users_id, array_keys($administrator_list))) { |
|
184 | if (is_object($this->forumObject)) { |
||
185 | $moderator_list = $this->forumObject->getVar('forum_moderator'); |
||
186 | } else { |
||
187 | $moderator_list = newbbIsForumModerators($member_list); |
||
188 | } |
||
189 | } |
||
190 | |||
191 | foreach ($users_online as $uid => $user) { |
||
192 | if (in_array($uid, $administrator_list)) { |
||
193 | $user['level'] = 2; |
||
194 | } elseif (in_array($uid, $moderator_list)) { |
||
195 | $user['level'] = 1; |
||
196 | } else { |
||
197 | $user['level'] = 0; |
||
198 | } |
||
199 | $online['users'][] = $user; |
||
200 | } |
||
201 | |||
202 | return $online; |
||
203 | } |
||
204 | |||
205 | /** |
||
206 | * Write online information to the database |
||
207 | * |
||
208 | * @param int $uid UID of the active user |
||
209 | * @param string $uname Username |
||
210 | * @param $time |
||
211 | * @param string $forum_id Current forum_id |
||
212 | * @param string $ip User's IP adress |
||
213 | * @param $topic_id |
||
214 | * @return bool TRUE on success |
||
215 | * @internal param string $timestamp |
||
216 | */ |
||
217 | public function write($uid, $uname, $time, $forum_id, $ip, $topic_id) |
||
266 | |||
267 | /** |
||
268 | * Garbage Collection |
||
269 | * |
||
270 | * Delete all online information that has not been updated for a certain time |
||
271 | * |
||
272 | * @param int $expire Expiration time in seconds |
||
273 | */ |
||
274 | public function gc($expire) |
||
283 | |||
284 | /** |
||
285 | * Get an array of online information |
||
286 | * |
||
287 | * @param CriteriaElement $criteria {@link CriteriaElement} |
||
288 | * @return array Array of associative arrays of online information |
||
289 | */ |
||
290 | public function getAll(CriteriaElement $criteria = null) |
||
315 | |||
316 | /** |
||
317 | * @param $uids |
||
318 | * @return array |
||
319 | */ |
||
320 | public function checkStatus($uids) |
||
348 | |||
349 | /** |
||
350 | * Count the number of online users |
||
351 | * |
||
352 | * @param CriteriaElement $criteria {@link CriteriaElement} |
||
353 | * @return bool |
||
354 | */ |
||
355 | public function getCount(CriteriaElement $criteria = null) |
||
368 | } |
||
369 |
The PSR-1: Basic Coding Standard recommends that a file should either introduce new symbols, that is classes, functions, constants or similar, or have side effects. Side effects are anything that executes logic, like for example printing output, changing ini settings or writing to a file.
The idea behind this recommendation is that merely auto-loading a class should not change the state of an application. It also promotes a cleaner style of programming and makes your code less prone to errors, because the logic is not spread out all over the place.
To learn more about the PSR-1, please see the PHP-FIG site on the PSR-1.