User controller with method to display user profiles

Question

I have few questions. Sorry if title is not a good fit. I could not think of a good one. I have read online about maintainability and elegance of the code. I have also seen few examples of DRY principal. I would like to ask

What is maintainability in PHP perspective? How to make code more elegant few examples will be appreciated? Finally How to use DRY principal in best possible manner?

I have a code piece which I would like to make maintainable, elegant and run DRY principal on it if possible. Sorry Its been years, I have not done coding. Looking for clear and concise answers.

Here is my code:

<?php

/**
 * I have used this function because I am not Allowed to 
 * Change UserService.php Otherwise using namespaces is best way.
 */
require_once("UserService.php");


class UserController extends UserService {


    /**
     * This function gets the values
     * like name, surname and date
     */
    public function showUserAction() {

        //this checks Whether the request if Post or not and dealt according.
        try {
            $is_active  = htmlspecialchars(isset($_POST["active"]) ? $_POST["active"] : NULL);
            $from       = strtotime(isset($_POST["from"]) ? $_POST["from"] : NULL);
            $to         = strtotime(isset($_POST["to"]) ? $_POST["to"] : NULL);
            $name       = htmlspecialchars(isset($_POST["name"]) ? $_POST["name"] : NULL);
            $surname    = htmlspecialchars(isset($_POST["surname"]) ? $_POST["surname"] : NULL);
            $sort_by    = isset($_GET["sortby"]) ? $_GET["sortby"] : NULL;
            $sort_col   = isset($_GET["sortcol"]) ? $_GET["sortcol"] : NULL;
            //retrieving the data from parent 
            $user_data = $this->getAllUsers();
            $sort = $sort_by === 'asc' ? 'desc' : 'asc';
            if($sort_by != NULL && $sort_col != NULL) {
                // according to my defined behavior when ASC will be passed it will sort the array in ascending order and in reverse order if the DESC is passed
                usort($user_data, fn($a, $b) => 
                //strnatcmp: compares the strings in natural order and it is Case Sensitive
                    $sort_by === 'asc' ? strnatcmp($a->{$sort_col}, $b->{$sort_col}) : strnatcmp($b->{$sort_col}, $a->{$sort_col}));
            } 
            //printing the html, if this is not done. I could have to use O(n2). which is worst case scenario
            $table =    "<table border='1'>" .
                        "<tr>" . "<th>" . "ID" . "</th>".
                        "<th>" . "<a href='UserController.php?sortby=".$sort."&sortcol=name'>Name</a>" . "</th>" .
                        "<th>" . "Surname" . "</th>" .
                        "<th>" . "Active" . "</th>" .
                        "<th>" . "Last Login" . "</th>" .
                        "<th>" . "Picture" . "</th>" .
                        "<th>" . "<a href='UserController.php?sortby=".$sort."&sortcol=rating'>Rating</a>" . "</th>" . "</tr>";
            echo $table;
            //showing the data in rows
            foreach ($user_data as $data) {
                echo "<tr>" . "<td>" . $data->id . "</td>";
                echo "<td>" . $data->name . "</td>";
                echo "<td>" . $data->surname . "</td>";
                echo "<td>" . $data->active . "</td>";
                echo "<td>" . $data->last_login . "</td>";

                $filename = $data->picture;
                //Set width and height of the picture
                $width = 100;
                $height = 100;

                //get dimensions of the picture
                list($width_orig,$height_orig) = getimagesize($filename);
                $ratio_orig = $width_orig/$height_orig;

                if ($width/$height > $ratio_orig) {
                $width = $height*$ratio_orig;
                } else {
                $height = $width/$ratio_orig;
                }

                //Resample the image
                $image_p = imagecreatetruecolor($width,$height);
                $image = imagecreatefromjpeg($filename);
                imagecopyresampled($image_p, $image, 0, 0, 0, 0, $width, $height, $width_orig, $height_orig);

                // storing the images in thumbnails directory for reusability
                $thumb = 'data/thumbnails/'.basename($filename);
                imagejpeg($image_p,$thumb , 100);
                echo "<td>" . "<img src=". $thumb . ">" . "</td>";
                imagedestroy($image_p);
                imagedestroy($image);
                echo "<td>" . $data->rating . "</td>" . "</tr>";

                //show the images, name, surname and last_login
                echo "<div>". "<img src=". $thumb . ">" .
                "<p>Name: " . $data->name . "</p>" . 
                "<p>Surname: " . $data->surname . "</p>" .
                "<p>Last Login: " . $data->last_login . "</p>"   
                ."</div>";
            } 
        } catch (Throwable $t) {
            echo "The Following Errors must be solved : " . $t->getMessage(); 
        }  
    }
}

$users = new UserController();
$users->showUserAction(); 

Answer 1

• In your posted script, the following variables have no purpose and should be removed: $is_active, $from, $to, $name, $surname. This prevents me needing to explain why calling htmlspecialchars() on a null value is meaningless.
• The $v = isset($v) ? $v : null pattern is replaceable with the null coalescing operator $v = $_GET['v'] ?? null;.
• Instead of $user_data, use a variable name that indicates that its payload will contain multiple users' data, perhaps $all_users.
• Loose comparisons like $sort_by != NULL can be simplified to $sort_by in the condition which is also a loose, falsey comparison.
• I don't know what the $sort_col values are like, but you may want to try the replacement of iterated strnatcmp() calls with the spaceship operator -- $a->{$sort_col} <=> $b->{$sort_col}. If this lets you down, then stay with strnatcmp().
• Rather than re-calling strnatcmp() to do the reverse sorting direction, you can declare a variable to hold 1 for asc and -1 for desc, then multiply the integer returned by the three-way comparison by this factor to adjust the sorting direction.
• I think that $sort should be better named $toggled_sort because you are inverting the value to be used in the html links to be later rendered.
• You can build potentially shorter hyperlinks by not requiring the ascending sorting direction to be declared in the url. If there is a sorting column and no sorting direct, assume ascending sorting.
• You may be only quickly drafting this script, but the border attribute is now an obsolete attribute and should be replace with proper css styling.
• I find it to be unpredictable in terms of UX, then if I sort on one column, then decide to sort on a different column -- that column's sorting direction will be the opposite of the first column's sorting direction. In other words, whenever a new column is to be sorted, it should default to ascending. This means you will need to build this logic into the code.
• To avoid messy concatenation while building your HTML markup, use template strings with placeholders so that you can printf(). This will keep your script very readable and easier to maintain.
• You should DEFINITELY not be resizing all of your users' images on every display of your users. The thumb creation should be done only once -- at time of upload. You do not want to be exhausting resources and slowing down page load times by adding this action in this script.
• As a overarching critique, I do not like the generation of HTML within a controller. HTML generation is the role of the view. This means we should (almost) never see echoing from a controller.
• If you want to receive POSTed data somewhere, handle it with a script that is not decided to display users. Property separation of responsibilities will help you to form more professional projects.
• It is invalid markup to jam a <div> inside of a <table> element (after the </tr> of each iteration), this needs to be redesigned.
• If you plan to use concatenation, there is no reason to concatenate static strings to static strings, just write them together and only concatenate when you have something non-static to append.

Here are some untested suggestions:

Controller:

public function showUsers(): void
{
    $all_users = $this->getAllUsers();
    $sort_col = $_GET["sortcol"] ?? null;
    $sort_by = $_GET["sortby"] ?? null;
    if ($sort_col) {
        $direction = $sort_by === 'desc' ? -1 : 1;
        usort($all_users, fn($a, $b) => $direction * ($a->{$sort_col} <=> $b->{$sort_col}));
    }
    sendToUserView(compact(['all_users', 'sort_col', 'sort_by']));
}

View:

$table_template = <<<HTML
<table class="border">
    <tr>
        <th>ID</th>
        <th><a href="UserController.php?sortcol=name%s">Name</a></th>
        <th>Surname</th>
        <th>Active</th>
        <th>Last Login</th>
        <th>Picture</th>
        <th><a href='UserController.php?sortcol=rating%s">Rating</a></th>
    </tr>
%s
</table>
HTML;

$row_template = <<<HTML
    <tr>
        <td>%d</td>
        <td>%s</td>
        <td>%s</td>
        <td>%s</td>
        <td>%s</td>
        <td><img src="%s"></td>
        <td>%s</td>
    </tr>
HTML;

$rows = array_map(
    $all_users,
    fn($user) => sprintf(
        $row_template,
        $user->id
        $user->name
        $user->surname
        $user->active
        $user->last_login
        $user->thumb
        $user->rating
    )
);

printf(
    $table_template,
    $sort_col === 'name' && $sort_by !== 'desc' ? '&sortby=desc' : '',
    $sort_col === 'rating' && $sort_by !== 'desc' ? '&sortby=desc' : '',
    implode("\n", $rows)
);