patternphpMinor
Review/rate my new graph class
Viewed 0 times
newgraphrateclassreview
Problem
I have written a PHP class called "graph". It is a class that performs RESTful-like commands to a MySQL database. I have posted the GitHub repo.
Here is the code as well:
config.php
database.class.php
```
"database",
"instrcut" => "dbConnect",
"status" => "fail",
"message" => mysql_error()
);
}
if($db == ''){
//sets the $db var to the constant MAIN_DB if not otherwise set.
$db = MAIN_DB;
$db_select = mysql_select_db($db);
}else{
$db_select = mysql_select_db($db);
}
if(!$db_select){
$response = array(
"object" => "database",
"instrcut" => "dbConnect",
"status" => "fail",
"message" => mysql_error()
);
}else{
$response = array(
"object" => "database",
"instrcut" => "dbConnect",
"status" => "pass",
"message" => "The database '$db' was connected to successfully."
);
}
return $response;
}
public static function conQuer($sql,$db){
$query = self::dbConnect($db);
$result = mysql_query($sql);
if($query&&$result){
$response = array(
"object" => "database",
"instrcut" => "conQuer",
"status" => "pass",
"message" => "The connection was established and the query was run successfully.",
"data" => $result
);
}else{
$response = array(
"object" => "database",
"instrcut" => "conQuer",
Here is the code as well:
config.php
database.class.php
```
"database",
"instrcut" => "dbConnect",
"status" => "fail",
"message" => mysql_error()
);
}
if($db == ''){
//sets the $db var to the constant MAIN_DB if not otherwise set.
$db = MAIN_DB;
$db_select = mysql_select_db($db);
}else{
$db_select = mysql_select_db($db);
}
if(!$db_select){
$response = array(
"object" => "database",
"instrcut" => "dbConnect",
"status" => "fail",
"message" => mysql_error()
);
}else{
$response = array(
"object" => "database",
"instrcut" => "dbConnect",
"status" => "pass",
"message" => "The database '$db' was connected to successfully."
);
}
return $response;
}
public static function conQuer($sql,$db){
$query = self::dbConnect($db);
$result = mysql_query($sql);
if($query&&$result){
$response = array(
"object" => "database",
"instrcut" => "conQuer",
"status" => "pass",
"message" => "The connection was established and the query was run successfully.",
"data" => $result
);
}else{
$response = array(
"object" => "database",
"instrcut" => "conQuer",
Solution
A few points:
-
Comments should mainly describe reasons for non-obvious decision. Most of your comments seem to just describe what the code below is doing which you can see by looking at the code. Example:
Those comments add no value. Furthermore reiterating what the code does in the comments will usually lead to comments conflicting with code once a few rounds of refactoring have taken place.
-
Consider dealing with error cases first to reduce nesting. This makes the code easier to read when there are multiple levels. E.g.:
In regards to the response refactoring:
Disclaimer: Above code might not be 100% correct but should get the idea across. It's been a long time since I worked with PHP.
- You should avoid
mysql_query. It's deprecated. Use PDO or mysqli.
- Read up on SQL injection attacks and use parametrized queries.
- Your indentation seems to be all over the place (not sure if that's a result of copy-n-paste, haven't checked the github sources)
- Use more spaces - they are cheap (e.g.
}else{should be at least} else {);
-
Comments should mainly describe reasons for non-obvious decision. Most of your comments seem to just describe what the code below is doing which you can see by looking at the code. Example:
//decode json data
$data = json_decode($jsonData);Those comments add no value. Furthermore reiterating what the code does in the comments will usually lead to comments conflicting with code once a few rounds of refactoring have taken place.
- Why is this called
Graph? Graph means usually one of two things to me: Either a data structure with nodes and edges connecting the nodes or the visualization of some data. I can see neither of those here really.
- You repeat a lot of string literals in your response creation. This should be refactored into a method. Makes refactoring later easier and avoids typos. Maybe even create a class encapsulating the response so you can do
r = response::build(...); r.Add(key, value); r.toJson();or something along those lines.
- Most prominently you use
instrcutin your database class butinstructin your graph class. I'd actually usecommand(instructions in computer terms are usually associated with more low level operations but YMMV)
-
Consider dealing with error cases first to reduce nesting. This makes the code easier to read when there are multiple levels. E.g.:
if (!$db || !$table)
{
return response::build(...).toJson();
}
if (!$fieldData)
{
return response::build(...).toJson();
}
// handle the main case
return response::build().toJson();In regards to the response refactoring:
class response
{
private data = array();
public static function build($object, $command, $status, $message)
{
$res = new response();
$res->data['object'] = $object;
...
return $res;
}
public function toJson()
{
return json_encode($this->data);
}
}Disclaimer: Above code might not be 100% correct but should get the idea across. It's been a long time since I worked with PHP.
Code Snippets
//decode json data
$data = json_decode($jsonData);if (!$db || !$table)
{
return response::build(...).toJson();
}
if (!$fieldData)
{
return response::build(...).toJson();
}
// handle the main case
return response::build().toJson();class response
{
private data = array();
public static function build($object, $command, $status, $message)
{
$res = new response();
$res->data['object'] = $object;
...
return $res;
}
public function toJson()
{
return json_encode($this->data);
}
}Context
StackExchange Code Review Q#32441, answer score: 2
Revisions (0)
No revisions yet.