Skip to content

Instantly share code, notes, and snippets.

@Trench94
Last active April 22, 2022 16:46
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save Trench94/c80e73fcb4b4855e021edcd91f894bae to your computer and use it in GitHub Desktop.
Save Trench94/c80e73fcb4b4855e021edcd91f894bae to your computer and use it in GitHub Desktop.
BookingManager class code review
<?php
/**
* The following code will be published to a production server.
*
* It's a simple script that allows a customer to view their booking status.
*
* Find as many errors or flaws as you can. You can edit the code inline, and/or
* leave a comment where you think you've found a flaw.
*/
// Any thoughts around PSR4 namespace and use of packages / other code?
// 1. Declaring strict types is a good thing, it means no type coercion allowed
// 2. This can be declared at your application entry point
declare(strict_types=1);
// 1. Error reporting should be set on your local machine not production
// 2. This can be achieved with your php.ini
// 2. If you need to do this, i wouldn't do this here. Do it at the entry point of your application.
error_reporting(E_ALL);
ini_set('display_errors', 'on'); // Not a good idea to enable this on production for security reasons
class BookingManager // Does this class extend a base class?
{
public string $db; // PDO does not return string type and don't make this a public variable
public function __construct() // You can use dependency injection here
{
// 1. 'root' and 'password' is a terrible set of credentials for a production environment, please change this.
// 2. Use a DB connector class to prevent connecting again and again to the DB and don't rely on hardcoded credentials
// 3. PHP 7.0 or above doesnt allow localhost so use 127.0.0.1 instead
// 3. What about exception handling? What happens if it fails to connect?
$this->db = new PDO('mysql:host=localhost;dbname=hp1066', 'root', 'password');
}
/**
* Get the status of a booking. The primary key for the `bookings` table is (booking_id, organisation_id).
*
* @param $bookingId
* @param $organisationId
* @return string|null // Update your PHPDocs
*/
private function getStatus($bookingId, $organisationId): ?string // Return type is string but function returns a boolean
{
// PDO::FETCH_BOTH results in both numeric and associative array indexes and gives duplicates
// Look at PDO::FETCH_ASSOC at the link below:
// https://phpdelusions.net/pdo/fetch_modes
$sth = $db->query('SELECT status FROM bookings WHERE organisation_id = ' . $organisationId, PDO::FETCH_BOTH);
// 1. $sth could be a more descriptive variable name
// 2. Wrap the foreach loop so we can see scope issues
foreach ($sth as $row) {
// booking_id can be a primary key of 'id' for better naming conventions
if ($row['booking_id'] == $bookingId) { // Why are you replicating what a query can do?
return $row['status']; // This return has limited scope
}
}
/* Returning the row status results in a boolean or integer depending on field type...
but you asking for a returned string */
return false; // A check on the query result should be done because $sth can return false
// REMEMBER:
/* Please use simple controllers with ideal naming conventions, don't mix return types.
Your controller is 'getStatus' not 'getStatusOrFalse' */
}
}
// 1. Move below into a different file and reference namespace
// 2. Instantiate this once in the constructor of a class
// 3. You can use a property in a class instead of local variable eg. $this->bookingManager
$manager = new BookingManager();
// 1. There is no validation or escaping of special characters done here, this is a huge security risk
// 2. getStatus is private and can't be accessed
// 3. Make this a class method so we can debug easily
$booking = $manager->getStatus($_REQUEST['bookingId'], $_REQUEST['organisationId']);
// 1. Don't do this inside a class file unless in a function, it prevents code being lost and keeps code tidy for others to use / understand
// 2. Use dump() or dd() instead of echo
echo 'Status: ' . $booking->status; // This can return false because of issues explained above
@Trench94
Copy link
Author

Trench94 commented Apr 19, 2022

This pull request to master will be rejected.

Please amend inline with the comments i have supplied and resubmit your amends for review.

NOTE: Test locally and remove debugging / error reporting before you resubmit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment