I opened up the source for Shiichan for kicks and I swear my eyes started bleeding. Not only is the source unbelievably messy (MIXING TABS AND SPACES SHOULD BE A CAPITAL CRIME), but it incorporates some of the stupidest coding practices I've ever seen.
Prime example of idiocy: foreach ($postarray as $apost) { list($start,$end) = explode('-',$apost); if (strpos($start,'l') === 0) {$start = ($numposts - intval(substr($start,1)))+1; $end = $numposts;} if ($start < 1) $start = 1; if ($end == "") if (strstr($apost,"-")) $end = $numposts; else $end = $start; if ($end > $numposts) $end = $numposts; if ($start > $numposts) $start = $numposts;> if ($start <= $end) { for ($i = $start; $i <= $end; $i++) {> list($name, $trip, $date, $message, $id, $ip) = explode("<>", $thread[$i]);> if ($trip) $trip = "#".$trip;> if ($isitreadphp) $return .= PrintPost($i, $name, $trip, $date, $id, $message, $postfile, 1, $boardname); else $return .= PrintPost($i, $name, $trip, $date, $id, $message, $postfile, $threadid, $boardname);> }> } else {> for ($i = $start; $i >= $end; $i--) { if ($end < 1) $end = 1; if ($start > $numposts) $start = $numposts; list($name, $trip, $date, $message, $id, $ip) = explode("<>", $thread[$i]); if ($trip) $trip = "#".$trip;> if ($isitreadphp) $return .= PrintPost($i, $name, $trip, $date, $id, $message, $postfile, 1, $boardname); else $return .= PrintPost($i, $name, $trip, $date, $id, $message, $postfile, $threadid, $boardname); } } }
Not >>6, but for starters I would break the code in many little functions. Instead of
if ($end > $numposts) $end = $numposts;
what about something like
$end = min($end, $numposts)
I don't know php (is this php?), maybe you could write something like
notMoreThan($end, $numposts)
or
makeBetween($end, 1, $numposts)
since there are some underchecks as well.
If you see, this function I'm proposing appears repeteadly in the code.
if ($start < 1) $start = 1;
if ($end > $numposts) $end = $numposts;
if ($start > $numposts) $start = $numposts; (which btw appears twice, god knows why)
if ($end < 1) $end = 1;
If I could figure out what the fuck it's trying to do I'd come up with a much better solution, but the near total lack of comments or documentation means I'd have to spend way more than I'm willing to spend to actually decipher this shit. I can tell you right now I'd use a database instead of a flat text file for this, probably powered by SQLite to retain the benefits of flat text storage. The query for that section of code would probably look something like this:
SELECT p.name, p.trip, p.date, p.message, p.postfile, p.ip,
p.threadid, b.boardname
FROM Posts p
JOIN Threads t ON (p.threadid = t.id)
JOIN Boards b ON (t.boardid = b.id)
WHERE p.threadid = ?
ORDER BY p.id ASC
>>7
How about using a better language. And making a separate function that parses the post query string into an array of post ids. And getting rid of the fucking duplicate code.
Oh fuck, OOP is so 90s! PrintPost($i, $name, $trip, $date, $id, $message, $postfile, 1, $boardname);