# Tag Info

69

Personally, I must say that close to all PDO derived/based classes I've seen so far suffer from the same problem: They are, essentially, completely pointless. Let me be clear: PDO offers a clear, easy to maintain and neat interface on its own, wrapping it in a custom class to better suit the needs of a particular project is essentially taking a decent OO ...

46

On a personal note, your code is really clean, the idea is brilliant, and I really hope to see it implemented soon. Your code is well implemented and structured, but there's syntax points that could be improved. I see a lot of basic if-else statements, if you're into ternaries, using them really slims down these statements, but it's at the cost of ...

28

I guess this is about as beautiful as PHP gets :p (I had not fun discovering this monstrosity.) One minor suggestion I have is to eliminate the duplication here: if (isset($question[$field_name])) { $sql_params[':' .$field_name] = $question[$field_name]; } else { $sql_params[':' .$field_name] = 0; } I'm surprised our good ...

19

Disclamer: My review will be short is longer than I expected but I will only focus on the function useData(). I've read it carefully and did my best to improve it and make it more readable for you. Lets get it started! The first thing that pops in my head is that giant pile of un-indented SVG: $svg = <<<END <svg xmlns="http://www.w3.org/... 12 Aliases Table aliases are handy, sure. But single-letter aliases are not good. It's OK to want to save having to type more characters than needed, but you have to keep in mind that things like aliases and variables get really confusing if the name you give it does not say anything about what it means. c, o, p, ot... Why not instead cust, ord, prod, ordItems? ... 12 It is a maxim in computer science that there are really "only three numbers": zero, one, and many. You have a case of many, and you should treat it as such. Otherwise, someday a customer might want to buy a hundred of some food item, and performance will be poor. Therefore, I recommend… Option four Instead of having the table represent current supply ... 11 Yes, there sure is. Effectively, what you seem to be doing can be reduced to this:$stmt = $dbh->query("SELECT * FROM " .$table); $data =$stmt->fetchAll(PDO::FETCH_ASSOC); echo json_encode($data); I'll explain how I got to those three lines above, simplifying your code step by step. The inner loop just does not make sense: foreach($val as $key2 =&... 10 You should not mix PHP logic and HTML. All logic should be placed on top of the document and HTML (+ some minor PHP loops/echos/ifs) below it;$var = $_GET['q']; is unneeded and you loose memory; never use the shorttag (<?); You forgot to set the errormode for PDO, that means it doesn't throw exceptions. To set it, use PDO::setAttribute:$pdo->...

10

Your code is fine. Really. However, it does show some lack of understanding the true benefits and power of prepared statements. I'm also not convinced you fully understand how the PDOStatement::bindParam method works. Allow me to explain: A prepared statement is sent to the DB server as a string, with the placeholders that will be replaced with the query ...

10

Yes, $category in your query string, coming directly from$_GET is a problem. You should validate $category, and that's not exactly the same as escaping. I assume you have a limited set of valid categories in your database, so validation should be something like:$valid_categories = array("sector", "industry", "company"); if (!in_array($category,$...

9

You properly protect against SQL injection, that's good. I worry about the filtering on the password. If I choose <script>' OR 1=1 ">>> as my password, the filtering should not reduce this to just ' OR 1=1 ", just because some part of it looks like HTML tags. A password is not supposed to be displayed anywhere, it is not supposed to ...

8

Besides the already mentioned things I like to add a few of my own: Connection charset You should set the charset of the connection the be the same of your php files (and definition when you use html): Simply add , array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES 'utf8'") after $this->pass Coding Style The naming of class variables shouldn't be ... 8 Poor cohesion It's called logical cohesion when a routine that does multiple things of different logic. Your function will either insert records, or select records, or update records. These are very different operations, and their implementations should be in separate functions. If you refactored this way, it would be much better: if ($Type == "INSERT") ...

8

From the things I see in your code it looks to me like you are quite new to Object Oriented Programming (OOP), so I will try to keep that in mind during this review. Basic OOP The __construct() method of a class is called only once per object of that class, when it is instantiated like so: $db = new Database(). Your class contains a field$db_connection, ...

8

I don't fully understand the desire for a "generic" data mapper. Oftentimes, flexibility leads to complexity and fragility in your application. Take for example your delete() method. It just arbitrarily accepts whatever object is passed and starts deleting things from the database. What is to stop someone from just creating a new object named similarly ...

7

When trying to make things like this, it's amazing how quickly it can become complicated and how many OOP ideas and principles can be involved. Anyway, there's a lot to cover here, so I'm just going to glaze over a few things and may come back and edit more detail in later. For a "first heavy OOP code design" this is pretty good (probably better than the ...

7

Kind of an old post, but I had to add something where I saw this line $this->datab->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);$this->datab->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); Which seems to me like you had hard-coding the settings, right after this line: $this->datab = new PDO("mysql:host={$host};...

7

If the second or later query fails you'll have inconsistent data in your database. Consider using atomic transactions. $query_2,$query_3 are bad names. You could pick something more descriptive, like $usersInsert,$freelancersInsert etc. $query =$this->db->prepare("INSERT INTO " . DB_NAME . ".users (firstname, lastname, email, email_code, ...

7

It's good that you're moving over. Many people think it's too difficult to switch over, but in the long run, it's definitely worth it. Technically you can assign \$user after you bind, because it's the actual execute call that substitutes. Although, in my opinion, it's easier to read when scanning the code to see the assignment before the binding. You could ...

7

Yes, this code is vulnerable to SQL injection, just like any dynamically composed SQL that is not properly quoted. The quoting mechanism for identifiers is database dependent, though. A bigger concern is how this situation came about in the first place. If you ever need to programmatically choose a column like that, your schema is probably poorly designed. ...

7

First of, good luck with your interview! Over thinking and not thinking enough Writing code is easy, but writing code that will last longer then a minute is hard. You obviously know how to code. And you have a notion of separation of concern. You comment your code (not 100%, but still. you comment). But still, your code is crap. Sorry, but it really is. You ...

7

Singleton I wouldn't call your Database class a singleton wrapper, as you can have multiple instances of it (not of the PDOObject field, but of the Database class). See here and here for actual singleton pattern. I don't think that your approach is necessarily bad though. Is this overkill? I think that you are a bit dishonest in your example. Your not ...

7

Aside from the point mentioned by Alienari there's a huge mistake in your design. A Model shouldn't inherit from a Connection. Usually OOP-CRUD is done in a fundamentally different way: Create and maintain a Connection Use said connection to run queries against a database Take the data from queries and put them into a Model These three steps are usually ...

7

A Vote should not have access to the database. It should be a dumb 'entity' with setters, getters and a constructor to make the required object a valid one. You should take a look at the Repository and Data Mapper patterns. The point is, the object you want to save should not know how it is saved - meaning that you can change where you want to save it's ...

6

I'll probably revisit this later, but for now: The exception catching should be done at either the model or model-using layer. I would probably let the exceptions bubble up through the model unless you want the model to be fairly high level. For the sake of abstracting the DB layer away from the model layer, I might consider wrapping the PDOException in ...

6

Though prepared statements do a pretty good job protecting your DB from injection attacks, it never hurts to perform some additional checks on the data you're receiving. For example: when a request is supposed to contain an email address, you might perform a SELECT query usign that email address, or insert it in your DB. If so, you should add this: if (!...

6

Your code isn't exactly terrible, but this is code-review, and good code review has to be harsh, so please keep in mind that I mean this in the nicest of ways. That said, there are some issues I'd like to address: Redundancies - DNRY: That old dead horse: Do Not Repeat Yourself. You have a lot of redundant code. Of course, you can get into code-golfing, and ...

6

Here are my recommendation, you could make sure users password have a minimum length. 8 alphanumeric characters is ideal If a users password is 1 char, it can easily be broken no matter how good the salt is move you database connection and setup into a different class: In the database class, create two separate functions. Does Email Exist and Return ...

6

I will apologize for the lack of formatting and using your code segments as straight example - but as I re-read your code I see my old self a whole lot and I don't consider myself any level near some of the coders here. However one important aspect to improvement is obviously trial and error and a lot of refactoring. So you asked about OOP going from ...

6

This is absolutely insecure. I will list only a handful of things which I think need to be absolutely rewritten. Do not use crypt. Instead, please use password_hash. Along with password_verify, your code could be simplified a great deal. If you're going to use PDO, use it. Do not mix in other database access modules (MySQL). Especially not MySQL, it is not ...

Only top voted, non community-wiki answers of a minimum length are eligible