2
\$\begingroup\$

I would like to receive input from more experience coders regarding the potential improvements this piece of code might have. I am new to PHP development and I am sure this code looks horrible behind the eyes of someone more competent. It works, but I think there's plenty of room for improvement.

I have an images folder with several sub-directories, and inside them, different images are stored. The goal of this PHP snippet is to output HTML code in this format for each folder and files in my images folder:

<section id='examplefolder' class='gallery'>
    <div id='categoryimg'>
        <p>examplefolder</p>
    </div>
    <div>
        <p>examplefilename</p>
        <img src='images/otros/examplefolder/examplefilename.jpg' alt='examplefilename'>
    </div>
    <div>
        <p>examplefilename2</p>
        <img src='images/otros/examplefolder/examplefilename2.jpg' alt='examplefilename'>
    </div>
    <!-- The snippet gets the rest of the files and places them under the folder name-->
</section>

Here's said snippet:

<?php
    $folders = glob('images/otros/*', GLOB_ONLYDIR);
    foreach($folders as $folder) {
        echo "<section id='".basename($folder)."' class='gallery'><div id='categoryimg'><p>".basename($folder)."</p></div><!-- SECTION -->".PHP_EOL;
            $files = glob("images/otros/".basename($folder)."/*.{jpg}", GLOB_BRACE);
            foreach($files as $file) {
                echo "<div>".PHP_EOL."    <p>".basename($file, ".jpg")."</p>".PHP_EOL."    <img src='$file' alt='".basename($file, ".jpg")."'>".PHP_EOL."</div>".PHP_EOL;
            }
        echo "</section>";
    }
?>

In particular, this one gets all folders with images inside images/otros.

  • Would you please offer any tips regarding best practices, or how to make my code look more readable?
  • Have I used any deprecated functions?
  • Is PHP_EOL the best way to add a newline character in order to make the output HTML look better?
\$\endgroup\$
1
\$\begingroup\$

Is PHP_EOL the best way to add a newline character in order to make the output HTML look better?

No, I don't think so. If you want your HTML nice on output, why not have it nice in the source code as well?

So instead of this:

echo "<div>".PHP_EOL."    <p>".basename($file, ".jpg")."</p>".PHP_EOL."    <img src='$file' alt='".basename($file, ".jpg")."'>".PHP_EOL."</div>".PHP_EOL;

have this:

echo "<div>
        <p>".basename($file, ".jpg")."</p>
        <img src='$file' alt='".basename($file, ".jpg")."'>
      </div>;

Have I used any deprecated functions?

I don't think so. But generally, you can see that yourself, when you enable warnings in development (don't forget to turn them of in production though).

Would you please offer any tips regarding best practices, or how to make my code look more readable?

  • You could extract code blocks to functions
  • You could use more whitespace (eg around .)
  • You could extract duplicate function calls to well-named variables
  • have constants such as fixed directory names in as few places as possibly, to make changing it easier

This would give you something like this. It's not perfect, but hopefully a good start:

echo formatHTMLforDirectory('images/otros/');

/**
TODO PHPDoc comment
*/
function formatHTMLforDirectory($basedirectory) {   
    $directories = glob("$basedirectory/*", GLOB_ONLYDIR); 
    $html = "";
    foreach($directories as $directory) {
        $directoryName = basename($directory);
        $html .= "<section id='$directoryName' class='gallery'>
                    <div id='categoryimg'>
                        <p>$directoryName</p>
                    </div>
                    <!-- SECTION -->";
        $files = glob("$basedirectory/$directoryName/*.{jpg}", GLOB_BRACE);
        $html .= formatHTMLforFiles($files);
        $html .= "</section>";
    }
    return $hmtl;
}

/**
TODO PHPDoc comment
*/
function formatHTMLforFiles(array $files) {
    $html = "";
    foreach($files as $file) {
        $filename = basename($file, ".jpg");
            $html .+ "<div>
                        <p>$filename</p>
                        <img src='$file' alt='$filename'>
                      </div>";
    }
    return $hmtl;
}

Misc

  • I always get nervous when data from the outside is echoed unencoded, as it can lead to XSS. If you allow users to upload image files, you are already vulnerable. But even if not, you may allow this in the future, which would make you vulnerable then.
  • You might want to think about using a templating engine such as twig. Mixing HTML and PHP often leads to hard to read code.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Oh wow, that was awesome :) I'll take note of every single thing. Regarding XSS, I'm implementing an OwnCloud installation as a frontend for my client to upload images and manage folders inside images. I'll have to test the whole thing afterwards. Thanks for the help and for the templating engine suggestion. \$\endgroup\$ – Fausto NA Jan 6 '16 at 12:08

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.