Cfrtjryk

I would like to receive some feedback on the code I am currently using to display x amount of images on my website per

$success = true;

$page = array_key_exists('page', $_GET) ? $_GET['page'] : 1;



$all_pictures = glob('crossles/*.jpg');



$total_images = 0;

foreach ($all_pictures as $img_total) {

    $total_images++;

}



$images_per_page = 30;



$max_pages = ceil($total_images / $images_per_page);



if ($page <= -1) {

    $success = false;

}



if ($page > $max_pages) {

    $success = false;

}

if ($success === false) {

    echo "<div class='container text-center'><p class='text-danger fs-32 mt-100'>No pictures to be displayed.</p></div>";

}

I know that $_GET is not the safest way of doing this but since it is only to retrieve images I don't think it is that big of an issue.
It would be nice if I could get some feedback to implement it to my code if needed.

If you are concerned about using GET data then consider using POST data. You could also consider using filter_input() or filter_var() with FILTER_VALIDATE_INT.

$total_images = 0;

foreach ($all_pictures as $img_total) {

    $total_images++;

}

This entire block could be replaced by a call to count($all_pictures)

$total_images = count($all_pictures);

And that variable could be eliminated by calling count() inline:

$max_pages = ceil(count($all_pictures) / $images_per_page);

if ($success === false) {

While this is totally fine, idiomatic PHP code simply checks for a value that equates to false, often on the side of brevity:

if (!$sucess) {

The conditionals could also be combined, eliminating the need for $success:

if ($page <= -1 || $page > $max_pages) {

    echo "<div class='container text-center'><p class='text-danger fs-32 mt-100'>No pictures to be displayed.</p></div>";

}

$total_images = 0;

foreach ($all_pictures as $img_total) {

    $total_images++;

}

This could be just

$total_images = ($all_pictures === false) ? 0 : count($all_pictures);

or

if ($all_pictures === false) {

    $total_images = 0;

    $success === false;

} else {

    $total_images = count($all_pictures);

}

The glob function returns an array, so you can just do the normal array operations on it. You only have to check for an error. I'm not quite sure what your original code would have done if there was an error. You might want to check later so you can say something like "Oops, there was an error looking for images!" rather than "No images".

if ($page <= -1) {

This is a weird way to write this. First, what would you do if page were set to -.5 here? Second, what does a page value of 0 mean? You are defaulting to 1, so it would seem that you are 1-indexed. So why accept anything less than 1? Consider

if ($page < 1) {

That eliminates all the invalid values below the range.

For much the same reason,

$page = array_key_exists('page', $_GET) ? $_GET['page'] : 1;

should probably be

$page = array_key_exists('page', $_GET) ? intval($_GET['page']) : 1;

Get parameters can be user input, so you should sanitize them to what you expect.

    echo "<div class='container text-center'><p class='text-danger fs-32 mt-100'>No pictures to be displayed.</p></div>";

This is an odd way to write a string in PHP. Using double quotes indicates that there is a value to interpolate in it. But you don't have one here. This could just as well be

    echo '<div class="container text-center"><p class="text-danger fs-32 mt-100">No pictures to be displayed.</p></div>';

or

?>

    <div class="container text-center">

      <p class="text-danger fs-32 mt-100">No pictures to be displayed.</p>

    </div>

<?php

Consider moving the whole thing into a function or even a class.

class Page_Handler {



    private $image_count;



    private $success = true;



    private $all_pictures;



    private $max_page_number;



    private $page;



    private $images_per_page = 30;



    function __construct($path = 'crossles/*.jpg') {

        $this->all_pictures = glob($path);

        $this->success = $this->parse();

    }



    private function parse() {

        if ($this->all_pictures === false) {

            return false;

        }



        $this->image_count = count($this->all_pictures);

        $this->max_page_number = ceil($this->image_count / $this->images_per_page);



        $this->page = array_key_exists('page', $_GET) ? $_GET['page'] : 1;

        if ($this->page < 1 || $this->page > $this->max_page_number) {

            return false;

        }



        return true;

    }

There would also be getters and possibly other helper methods. I'll leave those as an exercise for the reader (mostly because I'm lazy). Note that you could differentiate between a failure to load the images and an invalid page. Those are two separate problems and you may want to message them differently, as one is a server error and the other is user error. Page number out of range is often a problem that the user can fix.

I changed the names of some of the variables. I generally use a system where plural names indicate collections and singular names indicate scalars. You don't have to follow the same system, but it's the one I use when writing code.

One of the differences here is that the parse code doesn't have to keep processing after an error. At the first error, it returns failure.