Hot answers tagged

68

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 ...


45

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 ...


27

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, $...


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, ...


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

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

Ok, it's a huge work, so, I'll try to do this by iteration, adding then when I've a little time. I'll start with the form. Iteration 1 Readability: Thereafter is the code with some improvements about it's readability. I've limited the width to 78 columns, use 2 spaces soft tabs instead of 4, add a line feed (and indentation) before {, etc. IMHO, 2 ...


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

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 ...


6

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. ...


6

I am building a store. When a user decides to make a purchase I need to store it in the database but since the purchase might be of more than 1 item I'm wondering which way is the best to insert multiple rows, here are my 3 options. Who said you had to insert multiple rows? If I buy 10 apples, that's one row: user_id = 42, food_id = 103, quantity = 10. ...


6

Wrapping PDO: General Approach Generally, wrapping PDO without actually adding any functionality is not recommended. See for example here, or pretty much any answer on basic PDO wrapper classes. Because what happens is that you take a class with a pretty nice (and well known) interface, and then reduce the power of that interface (you can only perform ...


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