2
\$\begingroup\$

I've created something that I called System, where you can login, create the files and delete the files.

It's not any kind of big project, but it's the biggest project I made in PHP OOP so far. It uses MySQL PDO database connection. Passwords are not hashed since there's no registration page. The thing I'm not sure about is that I know what is and how to use abstract classes, final classes etc. but I don't see opportunity or need to use it in the current script. I can barely imagine a situation where something like abstract class would be useful, so if you can, tell me where could I use some of this more advanced object-oriented stuff so it'll be useful.

All the files: https://github.com/quono/oopsystem

There's also an .sql file with ready-to-import database with a few examples.

It asks me to add some code here, so I put here all 3 main pages, includes are available on github.

index.php

<?php

session_start();

require 'includes/InputValidator.class.php';
require 'includes/Client.class.php';
require 'includes/User.class.php';
require 'includes/db.php';

$client = new Client();
if ($client->isLogged()) $client->redirect('system.php');

if ($_SERVER['REQUEST_METHOD'] === 'POST') {

    $nick = htmlspecialchars($_POST['nick']);
    $pass = $_POST['pass'];

    $inputValidator = new InputValidator($nick);
    if ($inputValidator->length(3, 20)) {
        $inputValidator = new InputValidator($pass);
        if ($inputValidator->length(8)) {
            $user = new User($pdo);
            if ($user->login($nick, $pass)) {
                $client->setAsLogged($user->getId($nick), $nick);
                $client->redirect('system.php');
            }   
        }
    }
    if (!isset($err)) $err = 'Nickname or password incorrect';
}
?>


<?php require 'layout/header.html' ?>


<h1>Sign in</h1>

<form method="post" action="<?=$_SERVER['PHP_SELF']?>">

    <p>Login: <input type="text" name="nick"></p>
    <p>Password: <input type="password" name="pass"></p>
    <input type="submit" value="Submit">

</form>


<?php if (isset($err)) echo '<p>' . $err . '</p>'; 

require 'layout/footer.html';

?>

system.php

<?php

session_start();

require 'includes/Client.class.php';
require 'includes/User.class.php';
require 'includes/Files.class.php';

$client = new Client();
if (!$client->isLogged()) if ($client->isLogged()) $client->redirect('index.php');

if (isset($_GET['logout'])) {
    $client->logout();
    $client->redirect('index.php');
}

require 'includes/db.php';
$files = new Files($pdo, $_SESSION['userid']);

if (!isset($_SESSION['filesData'])) $_SESSION['filesData'] = $files->fetch();

$htmlFileContent = '';

if (isset($_GET['open'])) {

    if (in_array($_GET['open'], array_column($_SESSION['filesData'], 'id'))) {  
        $file = $_SESSION['filesData'][array_search($_GET['open'], array_column($_SESSION['filesData'], 'id'))];
        $htmlFileContent .= '<hr>';
        $htmlFileContent .= '<p><b>' . $file['name'] . '.txt</b></p>';
        $htmlFileContent .= nl2br($file['text']);
    } else {
        $htmlFileContent = 'File doesn\'t exist on your system';
    }
}

if ($_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['delId'])) {
    if (in_array($_POST['delId'], array_column($_SESSION['filesData'], 'id'))) {
        $files->remove($_POST['delId']);
        $_SESSION['filesData'] = null;
        $_SESSION['message'] = 'File has been removed successfully';
        $client->redirect($_SERVER['PHP_SELF']);
    }
}


$htmlFiles = '';

foreach($_SESSION['filesData'] as $file) {
    $htmlFiles .= '<form action method="post">';
    $htmlFiles .= '<p><span style="margin-right:15px;"><input type="submit" name="delete" value="Remove"></span>';
    $htmlFiles .= '<a href="?' . 'open=' . $file['id'] . '">' . $file['name'] . '.txt</a></p>';
    $htmlFiles .= '<input type="hidden" name="delId" value="' . $file['id'] . '">';
    $htmlFiles .= '</form>';
}

if (empty($htmlFiles)) $htmlFiles = '<i>You don\'t have any files yet</i>';


?>

<?php require 'layout/header.html' ?>

<h1>System - manage your files</h1>

<?php 

echo 'Logged as: ' . $_SESSION['nick'];
if (isset($_SESSION['message'])) {
    echo '<p>' . $_SESSION['message'] . '</p>';
    $_SESSION['message'] = null;
}

?>

<p>
    <a href="add.php">Add new file</a>
</p>
My files:
<br>
<?=$htmlFiles?>

<p>
    <a href="?logout">Log out</a>
</p>

<?=$htmlFileContent?>

<?php require 'layout/footer.html'; ?>

add.php

<?php

session_start();

require 'includes/InputValidator.class.php';
require 'includes/Files.class.php';
require 'includes/Client.class.php';
require 'includes/User.class.php';

$client = new Client();
if (!$client->isLogged()) $client->redirect('index.php');

if ($_SERVER['REQUEST_METHOD'] === 'POST') {

    $name = htmlspecialchars($_POST['name']);
    $content = htmlspecialchars($_POST['content']);

    $inputValidator = new InputValidator($name);
    if ($inputValidator->length(1, 50)) {
        $inputValidator = new InputValidator($content);
        if ($inputValidator->length(0, 10000)) {
            require 'includes/db.php';
            $files = new Files($pdo, $_SESSION['userid']);
            $files->add($name, $content);

            if (!isset($err)) {
                $_SESSION['message'] = 'File has been added successfully';
                $_SESSION['filesData'] = null;
                $client->redirect('system.php');
            }

        } else {
            $err = 'Your file size is too big';
        }

    } else {
        $err = 'File name length is not correct';
    }
}

?>

<?php require 'layout/header.html'; ?>

<h1>Add new file</h1>
<form action="<?=$_SERVER['PHP_SELF']?>" method="post">
    <p>Name:<br><input type="text" name="name" maxlength="50"></p>
    <p>Content:<br><textarea cols="60" rows="15" name="content"></textarea></p>
    <p><input type="submit" value="Save"></p>
    <a href="system.php">Cancel</a>
</form>

<?php if (isset($err)) echo '<p>' . $err . '</p>'; ?>

<?php require 'layout/footer.html'; ?>
```
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Well, let me tell you the truth: this is not OOP at all. This is a very common approach called "ole good procedural disguised as OOP". Your code is essentially procedural. Class methods here are used as functions. You can rewrite them as functions and your code will be the same.

And even such a makeshift OOP is inconsistent.

  • for example, new Files($pdo, $_SESSION['userid']); - why instead of using $client a direct access to the session variable?
  • or, I would make the whole authentication into a method of the Client class, and call this method, like

    if ($_SERVER['REQUEST_METHOD'] === 'POST') {
        $client = new Client();
        $client->authenticate($pdo, $_POST['nick'], $_POST['pass']);
        $err = 'Nickname or password incorrect';
    }
    
  • the same goes for the logout code

To be honest, in your place I would rather try to learn how to make a more organized procedural code. For example instead of such "accordion" as in your add.php why not to make it more level

$err = [];
$inputValidator = new InputValidator($name);
if (!$inputValidator->length(1, 50)) {
    $err[] = 'File name length is not correct';
}
$inputValidator = new InputValidator($content);
if (!$inputValidator->length(0, 10000)) {
    $err[] = 'Your file size is too big';
}
if (!$err) {
    require 'includes/db.php';
    $files = new Files($pdo, $_SESSION['userid']);
    $files->add($name, $content);
    $_SESSION['message'] = 'File has been added successfully';
    $_SESSION['filesData'] = null;
    $client->redirect('system.php');
}

And you also need learn the basic PHP syntax. For example, why $stmt->fetchAll()[0]['id']; when there is fetch(), or even fetchColumn() intended exactly for this purpose? Both fetchAll() and fetch() would raise an error when there is no such user found, where fetchColumn() will just return FALSE.

Oh - and you really, really should learn basic SQL as well.

 $stmt->bindValue(':id', rand(10000001,99999999) . rand(10000001,99999999));

is abaslutely not how it works. You've got to make the id field auto_increment PRIMARY KEY in the table, and it will be assigned automatically and without the risk of collisions.

Your idea on using exceptions is also wrong. As well as on the variable scope. To catch every exception on the spot is not how exceptions are intended to work. Least there is any point in assigning the error message to a variable that will be discarded the next instant. In PHP, a variable inside a function is only alive a long as the function is executed and when the function is finished your $err variable gets in to the void. And therefore it makes no sense to check if (!isset($err)) - it is never set. In general, you should leave error and exceptions alone, and handle them in a single place with a dedicated code, instead of littering your application code with thousands of catch blocks. Here is an article I wrote on PHP error reporting.

Don't be discouraged though. OOP is a hard tack by itself, but tenfold when compared to obscene simplicity of traditional PHP. OOP is not a syntax. It's a mindset. No wonder you don't know what an abstract class is for, simply because you don't know yet what a regular class is for either. It takes time to realize.

To me, OOP is an approach that improves the manageability of the code at the cost of adding extra complexity. Which means that as long as your procedural code is manageable, you won't see any benefit from OOP, yet your code will become unnecessarily more complex.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for your answer. I would gladly modify my code, but I don't think it'd make any sense if that is not OOP at all. I don't know how I could make it more OOP, I followed tutorials, other people codes and that's what I Iearnt. I'm going to read your article about error handling. But the thing I need is to see a perfect code of a small project, similar to mine, where OOP was used well... You say that I use class methods as functions, it's true, how are they supposed to be used? \$\endgroup\$ May 9, 2020 at 9:44
  • \$\begingroup\$ For example, there are private methods that perform a lot of inner workings hidden from the outside world. Or there is an inheritance that cannot be used in such a small project. But really, like I said, OOP is not as primitive and intuitive as traditional PHP. You cannot expect learning OOP by reading a few comments, can you? \$\endgroup\$ May 9, 2020 at 9:53
  • \$\begingroup\$ I can't build a good code without studying other codes either :) I know what private methods do, but I haven't used them because my methods are small enough (they do stuff like only add, remove etc.). I would use them if I had a bigger method, like authenticate() that you used. Is this how it should look like? One big method doing one big task using a few small private methods? And could you recommend me some place where I can study codes and sharpen my OOP skills in making a project? Some day, I will post on StackExchange a fully object-oriented code that'll be pleasant to read... \$\endgroup\$ May 9, 2020 at 10:11
  • \$\begingroup\$ I am sure you will! Instead of links, I would suggest some books. Martin Fowler's, Bob Martin's, Matt Zandstra's. But before that, you should really get more experience with PHP at whole, to face the problems these books are dealing with \$\endgroup\$ May 9, 2020 at 11:46
  • \$\begingroup\$ Thanks, I will consider buying one of those in the future! \$\endgroup\$ May 9, 2020 at 11:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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