3
\$\begingroup\$

There is a task to create site-blog on PHP. I have made some classes, but my Senior reviewed them and said that approach is bad and "best practicies" need to be used. What did he mean? Any advises on how to improve my code. There are classes for database, user, posts and archive. IDK how to specify my question, there is no error messages or smth, but what may be 'best practicies'?

Working with database

<?php
/**
* Class DB
*/

class DB
{
 private $source;

 function connect()
 {
   global $config;

   if(!is_null($this->source))
     return true;

   $this->source = mysql_connect($config['db']['host'], $config['db']['user'], $config['db']['pass']);
   if(!$this->source)
   {
     die('Could not connect: ' . mysql_error());
   }

   if(!mysql_select_db($config['db']['db'], $this->source))
   {
     die('Could not select: ' . mysql_error());
   }

   $this->q("SET NAMES 'utf8'");
   $this->q('SET collation_connection = "utf8_general_ci"');

   return true;
 }

 function q($query)
 {
   return mysql_query($query, $this->source);
 }

 function select($query)
 {
   $res = $this->q($query);

   if(!$res)
     return null;

   $result = array();
   while(($r = mysql_fetch_assoc($res)) !== false)
   {
     $result[] = $r;
   }

   return $result;
 }
}  

User

<?php

class User extends DB
{

  public function __construct()
  {
    session_start();
    $this->connect();
  }

  public function auth($login, $pass)
  {
    $login = htmlspecialchars($login);
    $pass = htmlspecialchars($pass);

    $auth = $this->select('select * from user where login="' . $login . '" and pass="' . $pass . '"');

    if(count($auth) > 0)
    {
      $_SESSION['authorized'] = 1;
    }
    else
      $_SESSION['authorized'] = 0;
  }

  public function checkAuth()
  {
    if($_SESSION['authorized'] == 1)
    {
      return true;
    }
    else
      return false;
  }
} 

For posting

<?php

class Post extends DB
{
 public $id = 0;
 public $params = array();

 public function __construct($id = 0)
 {
   $this->connect();

   $this->id = (int)$id;

   if($this->id > 0)
   {
     $this->params = $this->select('select * from post where id=' . $this->id);
     if(is_array($this->params))
       $this->params = $this->params[0];
   }
 }

 public function Add($params)
 {
   if($this->id != 0)
     return false;

   if(!$params['name'])
     return false;

   if(!$params['message'])
     return false;

   if($this->q("insert into post (name, date, message) values ('{$params['name']}', NOW(), '{$params['message']}')"))
     return true;

   return false;
 }

 public function Update($params)
 {
   if($this->id == 0)
     return false;

   if(!$params['name'])
     return false;

   if(!$params['message'])
     return false;

   if($this->q("update post set name = '{$params['name']}', message = '{$params['message']}' where id=" . $this->id))
     return true;

   return false;
 }

 /**   
  * @param string $select
  * @param string $where
  * @param string $order
  * @param int $limit
  * @param int $offset
  * @return self[]
  */
 public function GetList($select = '*', $where = '', $order = '', $limit = 0, $offset = 0)
 {
   $return = array();
   $res = $this->select('select ' .
     $select . ' from post ' .
     ($where ? ' WHERE ' . $where : '') .
     ($order ? ' ORDER BY ' . $order : '') . ' ' .
     ($limit ? ' LIMIT ' . ($offset ? $offset . ', ' . $limit : $limit) : '') // LIMIT 10;  LIMIT 50, 10
   );

   if(!$res)
     return $return;

   foreach($res as $item)
   {
     $return[] = new self($item['id']);
   }

   return $return;
 }

 public function Delete()
 {
   if($this->q("delete from post where id=" . $this->id))
     return true;

   return false;
 }


 public function getArchive()
 {
   $monthNames = array(1 => 'January', 2 => 'February', 3 => 'March', 4 => 'April', 5 => 'May', 6 => 'June', 7 => 'July', 8 => 'August', 9 => 'September', 10 => 'October', 11 => 'November', 12 => 'December');
   
   $archiveList = $this->select('SELECT DATE_FORMAT(date, "%Y") year, DATE_FORMAT(date, "%c") month from post group by year, month ORDER BY year desc, month desc');
   if(!empty($archiveList))
   {
     foreach($archiveList as $k => $item)
     {
       $archiveList[$k]['month_name'] = $monthNames[$item['month']];
     }

   }

   return $archiveList;
 }
} 

Posts archive

<?php
class Post extends DB
{
 public $id = 0;
 public $params = array();

 public function __construct($id = 0)
 {
   $this->connect();

   $this->id = (int)$id;

   if($this->id > 0)
   {
     $this->params = $this->select('select * from post where id=' . $this->id);
     if(is_array($this->params))
       $this->params = $this->params[0];
   }
 }

 public function Add($params)
 {
   if($this->id != 0)
     return false;

   if(!$params['name'])
     return false;

   if(!$params['message'])
     return false;

   if($this->q("insert into post (name, date, message) values ('{$params['name']}', NOW(), '{$params['message']}')"))
     return true;

   return false;
 }

 public function Update($params)
 {
   if($this->id == 0)
     return false;

   if(!$params['name'])
     return false;

   if(!$params['message'])
     return false;

   if($this->q("update post set name = '{$params['name']}', message = '{$params['message']}' where id=" . $this->id))
     return true;

   return false;
 }

 /**
  * get list of posts
  * @param string $select
  * @param string $where
  * @param string $order
  * @param int $limit
  * @param int $offset
  * @return self[]
  */
 public function GetList($select = '*', $where = '', $order = '', $limit = 0, $offset = 0)
 {
   $return = array();
   $res = $this->select('select ' .
     $select . ' from post ' .
     ($where ? ' WHERE ' . $where : '') .
     ($order ? ' ORDER BY ' . $order : '') . ' ' .
     ($limit ? ' LIMIT ' . ($offset ? $offset . ', ' . $limit : $limit) : '') // LIMIT 10;  LIMIT 50, 10
   );

   if(!$res)
     return $return;

   foreach($res as $item)
   {
     $return[] = new self($item['id']);
   }

   return $return;
 }

 public function Delete()
 {
   if($this->q("delete from post where id=" . $this->id))
     return true;

   return false;
 }

 public function getArchive()
 {
   $monthNames = array(1 => 'January', 2 => 'February', 3 => 'March', 4 => 'April', 5 => 'May', 6 => 'June', 7 => 'July', 8 => 'August', 9 => 'September', 10 => 'October', 11 => 'November', 12 => 'December');

   // get all years and month when posts were made
   $archiveList = $this->select('SELECT DATE_FORMAT(date, "%Y") year, DATE_FORMAT(date, "%c") month from post group by year, month ORDER BY year desc, month desc');
   if(!empty($archiveList))
   {
     foreach($archiveList as $k => $item)
     {
       $archiveList[$k]['month_name'] = $monthNames[$item['month']];
     }

   }

   return $archiveList;
 } 
``` 
\$\endgroup\$
1
  • \$\begingroup\$ Are you really using mysql_connect? What PHP version are you running? I am finding it hard to believe that you could write such code in 2020 \$\endgroup\$ – Dharman Nov 11 '20 at 18:26
6
\$\begingroup\$

I really hope that you are trolling us, but if you are not trolling and this is a real code, then I must say that it should absolutely never be used. This might have been acceptable 25 years ago, but even then there are too many issues with this code.

  • use of old mysql_* API (you know this API is not available anymore, right?)
  • SQL injection,
  • cleartext passwords,
  • HTML escaping the data going into the database,
  • NO utf8 support,
  • questionable returns
  • false polymorphism
  • long array syntax
  • implicit code blocks
  • unnecessary loops
  • poorly named methods
  • unnecessary classes
  • and more...

1. Using mysql_* in 2020

As of November 2020, the minimum PHP version you should be using is PHP 7.3. PHP 7.2 is still available but not for long.

Given that the mysql_* API has been removed a couple of years ago and is no longer available this leads me to think that you might still be using PHP 5. Don't!. You are risking security of not only your own system but your users as well.

Learn about PDO and start using it, preferably with some kind of DB abstraction layer. e.g. EasyDB

2. SQL injection

You must always bind the data separately. Never concatenate PHP variables into your SQL unless they have been validated against a pre-approved list or hardcoded.

It would be a waste of time to explain for a millionth time how to use prepared statements, so let me just link you to How can I prevent SQL injection in PHP?

3. Never store plaintext passwords

You should never ever store your users' passwords on the server. You may only store secure hashes such as the ones generated by password_hash()

4. Use htmlspecialchars() properly

It's in the name: HTML. You should only use this function when displaying the data inside of an HTML document. Learn about XSS and never store the output of htmlspecialchars() in your database; this can only damage your data.

5. UTF-8 support

Your application should support full UTF-8. Use utf8mb4 as your database charset and set it when connecting to your database.

Pay special attention to how your application handles strings, to make sure that you never confuse byte operations with character operations. Use mbstring extension of string manipulation.

5. When to use return

Use return when your function should return a value or make an early exit. Do not use it to return if the function was not provided valid arguments or something can't be executed! This is what exceptions are for. For example:

 public function Delete()
 {
   if($this->q("delete from post where id=" . $this->id))
     return true;

   return false;
 }

This function should not return anything. This function causes side-effects, not return data. If the execution of SQL failed then there should be an exception. With PDO and mysqli this is done automatically for you.

Also, don't repeat the same thing over and over. If you have multiple conditions why a function might return early then combine them into a single if statement.

6. What is polymorphism

Polymorphism is when a class can be extended into subclasses that are a special type of the main class. For example, Mercedes-Benz and Volvo truck are special types of Vehicle. User is not a type of database! Your user class should never extend the database. The pattern you are looking for is composition.

Use dependency injection for providing dependant classes. I would also advise using composer autoloading and DI container if you're not using them yet.

7. Learn new syntax.

Use short array syntax ([]) rather than the old one (array()). Stay on top of the new features. They have been added to PHP for your convenience so that the code can be kept cleaner.

On the topic of clean code: adopt a coding standard (I recommend familiarizing yourself with PSR) and format your code properly. Avoid implicit code blocks after if statements. Also, use type hinting.

Avoid unnecessary coding. Don't reinvent the wheel. PHP has so many array functions that if you have to do something with a loop, you should first check if PHP doesn't already have a function for it. Once you adopt PDO, check all the different fetch modes. They help to avoid unnecessary coding/loops in PHP.

8. Name stuff properly

Methods are actions. They perform some task. They should be named accordingly. What action is auth() or checkAuth()? The name should describe the action.

9 DB class

Why would you need such class? The only real method there is select() which is just three lines of PDO code. If you use proper DB abstraction layer this becomes a single line. There's no use for such class.

The subclasses are even worse. Why do you have a method such as GetList() that should abstract SQL, but yet it accepts SQL as a parameter. This makes no sense. Just stick to normal SQL and prepared statements. Absolutely, no reason to have such class in your code.

10 Comments

This is slightly controversial, but what are you trying to achieve with the doclocks? I know they can be handy when describing a complex method or one that takes complex parameters, but why does the method Post::GetList() have a description get list of posts? PHP lets you specify types for your arguments and return values. What purpose does the docblock server then?

There's plenty of other issues in that code. Your session handling is something that raises an eyebrow. The whole application logic is very muddy. It seems you are not using namespaces. Your methods signatures are very inconsistent (array in one place vs. named parameters) I could go on for quite some time, but I think your main problem is your PHP version.

Upgrade PHP to something reasonable, delete this code, and start fresh. But first, please take some courses in web development or spend a few weeks reading online materials and self-studying. Don't start developing code that could potentially be run online until you gain some experience with web development.

\$\endgroup\$

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.