0
\$\begingroup\$

I did a working CRUD abstracted class, and I am not sure that this is a good way for it, so I would like to hear your opinion.

protected function properties(){
    $properties = array();
    foreach (self::$table_fields as $db_field){
        if(property_exists($this, $db_field)){
            $properties[$db_field] = $this->$db_field;
        }
    }
    return $properties;
}

public function create(){
    $db = db::getConnection()->conn;
    $properties = $this->properties();

    $key = implode(",",array_keys($properties));
    $value = implode(",:",array_keys($properties));

    $sql = "INSERT INTO ".self::$table."(" . $key . ") ";
    $sql.= "VALUES (:" . $value . ")";
    $stmt = $db->prepare($sql);
    foreach($properties as $key=>$value){
        $stmt->bindParam($key,$value);
    }
    if($stmt->execute()){
        print_r($stmt);
        $this->id = db::the_insert_id();
        return true;
    }else{
        print_r($stmt);
        return false;
    }
    return $stmt;
}

This is working well: I am getting TRUE as response and it creates a user in database, but I am just not sure about it.

\$\endgroup\$
  • \$\begingroup\$ Just to be clear — what class is this, and what is db::? \$\endgroup\$ – 200_success Jan 25 '18 at 22:39
  • \$\begingroup\$ db is database and its calling from another class. it's static method, i have also auto loader. \$\endgroup\$ – Никола Р. Jan 25 '18 at 23:50
  • \$\begingroup\$ I know that db stands for "database", but what is that code? Mysqli? \$\endgroup\$ – 200_success Jan 26 '18 at 0:17
  • \$\begingroup\$ it is PDO github.com/anonunder/firstep/blob/master/singleton_db.php \$\endgroup\$ – Никола Р. Jan 26 '18 at 1:04
0
\$\begingroup\$
  1. Format all identifiers in order to avoid a syntax error. Assuming it's mysql, use backticks for the purpose:

    $key = "`".implode("`,`",array_keys($properties))."`";
    
  2. You are going to repeat the second half of the code in the every crud method. To avoid that, add a query() method to your db class

    public function query($sql, $params = []) {
        $stmt = self::getConnection()->conn->prepare($sql);
        $stmt->execute($params);
        return $stmt;
    }
    

    so you'll be able to make your crud methods much more concise.

    public function create(){
        $properties = $this->properties();
    
        $key = "`".implode("`,`",array_keys($properties))."`";
        $value = implode(",:",array_keys($properties));
    
        $sql = "INSERT INTO `".self::$table."` (" . $key . ") ";
        $sql.= "VALUES (:" . $value . ")";
        db::query($sql, $properties);
        $this->id = db::the_insert_id();
    }
    
  3. There is no point in returning true or false from such a method, the only reason for it to return false is a database error and such an error should be thrown in the form of Exception and handled elsewhere, making all this true false stuff rather useless.

  4. Your current setup is following Active Record pattern, making a data object to contain all the database interaction related code. Consider switching to Data Mapper pattern where you have two classes - your data class and a mapper that is responsible for all the database interactions.
\$\endgroup\$
  • \$\begingroup\$ 2. What about binding params? What is the difference between using query and bindparam? also, I already have that method. Thanks. \$\endgroup\$ – Никола Р. Jan 25 '18 at 23:52
  • \$\begingroup\$ Don't you realize that your method doesn't use a prepared statement? \$\endgroup\$ – Your Common Sense Jan 26 '18 at 8:11
  • \$\begingroup\$ It is using, it is just automatic and that is why I asked is that good way to do it. This is output: prntscr.com/i601uc \$\endgroup\$ – Никола Р. Jan 26 '18 at 17:14
  • \$\begingroup\$ From the link you posted above, it is clear that your db:query() does NOT use a prepared statement, so I have no idea what are you talking about. \$\endgroup\$ – Your Common Sense Jan 26 '18 at 21:29
  • \$\begingroup\$ $stmt = $db->prepare($sql); foreach($properties as $key=>$value){ $stmt->bindParam($key,$value); } $stmt->execute() This isn't prepared statement? I'm not executing the script from query method.. look my firstpost. \$\endgroup\$ – Никола Р. Jan 26 '18 at 21:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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