5
\$\begingroup\$

I create blog posts as blog1.php...blog20.php...blog500.php etc and redirect users from blog.php (code below) to the most recent blog post as determined by the blog post with the highest number in the filename.

Here's how I'm doing it, but I have the sense this could be much cleaner and more elegant:

$directory = "/var/www/html";
$scanned_directory = array_diff(scandir($directory), array('..', '.'));

$blogNumbers = Array();
foreach($scanned_directory as $file){
        if(preg_match("/blog(?<digit>[0-9]+)/", $file, $matches)){    
                foreach($matches as $match){
                                 if(is_numeric($match["digit"])) array_push($blogNumbers, $match["digit"]);
                                     //had to put this numeric check in because otherwise
                                     //script was pushing some 'b's onto array                                         
                                 }
        }

}

$currentBlog = max($blogNumbers);    
header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog'.$currentBlog);
die();

People are always complaining that bad programming is too easy in PHP, and I suspect what I've posted is just that easy bad programming that makes people mad. Can someone tell me how to make this more PHP-like and more elegant? I've worked much more in Python, but I'd like to be less of a hack in PHP.

\$\endgroup\$
  • \$\begingroup\$ I would urge you to consider another solution than pure file based. If you have access to a database you could make pagination easier. But the most profitable gain would be how easily you could store meta-data about each page and add/remove meta-data in the future. Hitting the file-system is expensive compared to a database. Just a suggestion :) \$\endgroup\$ – AnotherGuy Jul 16 '15 at 23:11
  • \$\begingroup\$ If you don't like databases you can use a simple switch statement: blog.php?post=17 and in the blog.php file you include $post=$_GET['post']; switch($post) case:1,...., case:17... \$\endgroup\$ – Paracosmiste Jul 16 '15 at 23:21
  • \$\begingroup\$ @AnotherGuy I agree with you, but I am too lazy to deal with this right now, and it's cheaper for me to use the file system than to pay for an RDS. \$\endgroup\$ – sunny Jul 16 '15 at 23:21
  • \$\begingroup\$ Maybe try to use: flatpress.org/home ? \$\endgroup\$ – gonzalon Jul 17 '15 at 4:38
7
\$\begingroup\$

One thing that makes me cringe is the indentation. It really is off.
I had to look 20 times to that stray brace to realize it isn't closing that if(in_numeric()).


Your casing is inconsistent. You have array() and Array(). Pease, use array() only.

Same goes for $scanned_directory, $blogNumbers, $scanned_directory and $currentBlog.
Pick one style and use it!


Same goes with quotes. You have single-quoted and double-quotes strings everywhere!

Just pick 1 style and use it! (My recommendation goes to single-quotes.)


You have the following line:

if(is_numeric($match["digit"])) array_push($blogNumbers, $match["digit"]);

You should always use braces. Always! There have been serious bugs on iOS for this.


Why do you have a die(); at the end? As soon as the script finishes, it will just exit. No need to have the die(); there.


You have the following line:

$directory = "/var/www/html";

That sounds like a constant. Try something like this:

define('DIRECTORY', '/var/www/html');

Or:

const DIRECTORY = '/var/www/html';

And use it like this:

$scanned_directory = array_diff(scandir(DIRECTORY), array('..', '.'));

Almost at the end, you have this:

$currentBlog = max($blogNumbers);    
header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog'.$currentBlog);

After you fix your variables' casing, you can just do like this:

header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog' . max($blog_numbers));

How I would write this:

define('DIRECTORY', '/var/www/html');

$scanned_directory = array_diff(scandir(DIRECTORY), array('..', '.'));

$blog_numbers = array();
foreach($scanned_directory as $file) {
    if(preg_match('/blog(?<digit>[0-9]+)/', $file, $matches)) {    
        foreach($matches as $match) {
            //had to put this numeric check in because otherwise
            //script was pushing some 'b's onto array  
            if(is_numeric($match['digit'])) {
                array_push($blog_numbers, $match['digit']);                                       
            }
        }
    }
}

header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog' . max($blog_numbers));

Outside this review, that foreach doesn't seem necessary. But I don't know the actual names.

\$\endgroup\$
  • 1
    \$\begingroup\$ thanks a lot for the comments. Can you clarify about the curly braces? This is the first I'm hearing about this. \$\endgroup\$ – sunny Jul 16 '15 at 17:50
  • \$\begingroup\$ @sunny Sure. You see that curly brace, right bellow the comment? That is the stray brace I'm talking about. And you're welcome, we are here to help you and to improve your code. \$\endgroup\$ – Ismael Miguel Jul 16 '15 at 17:53
  • \$\begingroup\$ I want to add a quick note that it's excusable to leave out braces if your if is entirely inline, i.e. the code is on the same line. However, it's preferred that this be used only for trivial statements (a return or a die, for example). \$\endgroup\$ – Schism Jul 16 '15 at 22:00
3
\$\begingroup\$

Doing what you are currently doing

In addition to the answer by Ismael Miguel, I would add a few things. First, I think that replacing scandir by glob will be beneficial for you. The main reason for that is that glob does support some basic regex, that you could use in the following way : $scanned_directory = glob($directory."/blog?*.php") ; . The wildcard is used just after the interrogation point because, as stated in the documentation :

The ? matches 1 of any character except a /
The * matches 0 or more of any character except a /

It means that you will get all files beginning by blog and ending by .php that have 1 character after blog (effect of the interrogation point) or more (effect of the wildcard). That way blog.php will not be part of the resulting array.

Another reason for using glob is this aspect :

GLOB_NOSORT - Return files as they appear in the directory (no sorting). When this flag is not used, the pathnames are sorted alphabetically

Well, if it's sorted, why would we even bother to loop over all elements ? Just use array_pop to get the last element of the array returned by glob and you're good to go.

A better way to do ?

A small paragraph about a better approach about what you're doing. The idea to create as many files as blog posts that there will be is pretty bad. The main reason is that if you want to modify anything on all your pages (like the design of your blog), you will have to edit all files containing blog posts, which could be very long, not to mention that you may forget to do so for some. You should use include to put your design on all pages (if you're not already doing it), but the better approach would be to have a single file, blog.php, that fetches blog posts from a database. It would be way easier to do such things, for example you could order your blog post by date of publication while fetching them in a single query.

\$\endgroup\$
  • \$\begingroup\$ Actually, glob doesn't support regex. It supports search patterns. But I agree with everything you said. \$\endgroup\$ – Ismael Miguel Jul 17 '15 at 18:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.