Last active
April 22, 2022 16:46
-
-
Save Trench94/c80e73fcb4b4855e021edcd91f894bae to your computer and use it in GitHub Desktop.
BookingManager class code review
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<?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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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