current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

The subject is this small project - PHP/MySQL Database wrapper

The three main goals of this projects are:

  • Easy way to map database columns to object properties
  • Simple solution to insert or update data
  • Query builder class which generate sql query (something like ActiveRecord in CodeIgniter)

Some examples:

class User extends stdClass {
    function getName() {
        return $this->first_name . " ". $this->last_name;
    }
}

class Post extends stdClass {

    /**
     * @var User
     */
    public $author;
}

DB::getInstance()->setFetchTableNames(1);
$sql = DB::getInstance()
    ->select("p.*, u.*")
    ->from("posts p")
    ->join("INNER JOIN users u USING(user_id)")
    ->where("u.user_id = ?", $user_id)
    ->orderBy("p.title");

$stmt = $sql->execute();

/* @var Post[] $post_collection  */
$post_collection = array();

while($post = $stmt->fetchInto(new Post, "p")) {
    $post->author = $stmt->fetchIntoFromLastRow(new User, "u");
    $post_collection[] = $post;
}

// Usage
foreach($post_collection as $post) {
    echo $post->author->getName();
}

More examples are available in readme file at GitHub - https://github.com/salebab/database

So, please review this classes and I will be very pleased if you can give me some comment.

share|improve this question

closed as off-topic by rolfl Jun 4 at 12:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must include the code to be reviewed. Links to code hosted on third-party sites are permissible, but the most relevant excerpts must be embedded in the question itself." – rolfl
If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer 1

up vote 1 down vote

Firstly, a bit of humorous wordplay: witch = itch = burn; "She's a witch! BURN HER!" I think you meant "which" in this context. Now that is out of my system, on to the review :)

Why are you extending the stdClass? This is the class reserved for typecasting variables as classes. It is just a wrapper that has no benefits of being extended from. If you are just trying to create a normal class then just drop the extends stdClass bit. Its unnecessary.

Additionally, where did properties $first_name and $last_name come from? They were not declared. This means they either are coming from the parent class, which is impossible as the stdClass is empty, or they are defaulting to the public scope. If you were extending another class and that property was defined there, this would be fine, but default properties are bad. Besides, I have heard rumors that it will be deprecated soon, so prepare for the worst and just manually define their scope.

class User {
    public
        $first_name,
        $last_name
    ;
    //etc...
}

What is the point of the Post class? Just to declare the $author property? This is unnecessary, especially for a full fledged class. The only time I could foresee something like this being ok is if it were part of an interface or abstract class, but then it should also declare the expected methods as well.

Why is your database class completely static? I understand adopting a singleton pattern to ensure only one connection can be made, but after that you should be able to use the rest of your class like normal. Instead you are calling it statically everywhere. This is what you should be able to do with the singleton pattern. Notice how you only have to get the instance once.

$db = DB::getInstance();
$db->setFetchTableNames( 1 );
$sql = $db->select(//etc...

Why are you looping over the same information twice? If you find yourself looping over something in order to create an array so that you can loop over it again, don't. Just do it all the first time.

while( $post = $stmt->fetchInto( new Post(), 'p' ) ) {
    $post->author = $stmt->fetchIntoFromLastRow( new User(), 'u' );
    echo $post->author->getName();
}

Now, I can't really help you much more because you've not posted all of your code, just examples. I can't review your class except in the abstract. If you want a more complete review, you should follow the FAQ and post all related code. For instance, you are asking about your database wrapper, but all you have posted are a couple of slightly related container classes and example usages. Where's the actually database wrapper?

share|improve this answer
    
The subject of question isn't provided code, that is just example, the subject is code on github. I didn't read the FAQ and recomendation to include the whole code here. There is so much code, I cannot do that. –  sasa Oct 2 '12 at 12:35
    
@sasa: You do not need to include ALL of your code, just the class in question. In this case that would be the DB class. The dependencies should be unnecessary as it should be obvious from the code what you are trying to do, if not, that will just be something I will point out. I wouldn't mind taking a look at this on github later, but I can't while I'm at work and I have classes in the afternoon, so I'm not sure when I could get around to it. –  mseancole Oct 2 '12 at 13:06

Not the answer you're looking for? Browse other questions tagged or ask your own question.