HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Review/rate my new graph class

Submitted by: @import:stackexchange-codereview··
0
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",

Solution

A few points:

  • 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 instrcut in your database class but instruct in your graph class. I'd actually use command (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.