|
warning: typical HN nitpicking (I know, typical, but it was just my first reaction upon seeing the code & I want to be honest). I looked at the code and my first impression wasn't good. These are minor issues but such "smells" are red flags (to mix metaphors) and warn me away from the codebase https://github.com/circa75/dropplets/blob/master/index.php#L... } else if($_GET['filename'] == 'rss' || $_GET['filename'] == 'atom') {
why would you set a get var called "filename" to request an RSS feed? There is no actual file called `rss`, so it's not a file name. So the filename get var is really maybe a file name, maybe some other non-file resource being requested. This is confusing & unintuitive, makes the code harder to read & parse.https://github.com/circa75/dropplets/blob/master/index.php#L... $posts = ($pagination_on_off != "off") ? array_slice($all_posts,$offset,($posts_per_page > 0) ? $posts_per_page : null) : $all_posts;
too many ternaries!! Why are you testing against a specific string ("false") to indicate a boolean value? This is a major smell as it suggests the author doesn't know the proper use of basic PHP data types.* Why are you adding a ternary if to test posts_per_page size rather than just setting it to `null` to indicate "all posts" in the first place? How can pagination be true with unlimited posts per page anyway? Make posts_per_page null by default, remove redundancy in pagination config var name, make it a bool, and you're left with: $posts = $paginate ? array_slice($all_posts, $offset, $posts_per_page) : $all_posts;
I don't mean to condemn the efforts of the author: everyone starts somewhere and this looks like a pretty dang cool project, but smells like these make me walk away immediately because they indicate this code is going to be confusing and hard to work with. One last one: functions.php. This will send a cold chill down the spine of any PHP dev familiar with the bad old days. Group code logically, write reusable modules, and use namespaces (well skip this last one if you want to support old/cheap hosting)! No need to code like its PHP 4 anymore :)* Yes I see that it's a user defined setting in settings.php or config.php and an argument could be made that a user would understand "on"/"off" more easily than true/false, but this is a trivially solved by a comment & would make the code more robust and readable. More robust because string comparison in PHP is case sensitive, so if the user sets the setting to "OFF" it will effectively be "on." Boolean solves this. |
I actually don't remember why I wrote them like that, but I'm pretty sure I had a reason (at least I think I do..)
Again, sorry about that.