views:

23

answers:

3

This is my first time. I will appreciate any thoughts, tips, and what not. How can I improve this? Ultimately, I don't want so many selects in my script. I don't know what should a MySQL type of function consist of. Any inputs on what I should include would be great also, such as mysql_query().

function mysqlSelectCodes($table, $where, $order, $limit) { $sql = "SELECT * FROM $table WHERE $where ORDER BY $order LIMIT $limit" or die(mysql_error());

}

+1  A: 

This code wil actually to nothing. You're just building a query string but you don't execute the query.

If no additional measures are in place, this code is highly prone to SQL Injection attacks.

You may want to read the PHP manual for PDO. This will help you.

Techpriester
+1  A: 

It's good you're asking for help, please don't let what I'm about to say discourage you--keep working at it! There are a lot of issues with this, so I'll just toss out a few:

  1. this doesn't really do anything because it doesn't return anything (if $sql is global, it shouldn't be)
  2. en.wikipedia.org/wiki/SQL_injection
  3. or die(...) wouldn't belong in a string builder like this--it's not going to handle SQL failures here since all it's testing for is if the given line succeeds.
Michael Haren
Thanks, so I'm guessing it's better to just have a lot of hardcoded selects? I want to kind of clean it up.
Doug
Building a library, which contains your queries is good, that's not what you're doing here. Please, please look into SQL injection attacks--that's the first thing you should do. Go and convert your queries to be parameterized. They might be a little faster (though probably not noticeably) and more importantly, they will be a lot safer
Michael Haren
+1  A: 

My advice: I probably wouldn't bother to do this. Apart from the specific problems others have pointed out (SQL injection, forgot to return), there's a bigger issue I have with this: you're assuming that all queries will fit nicely into this form. Limitations of your function include:

  • No support for joining multiple tables.
  • No support for inner selects.
  • You always select all columns using * - a bad idea in general. You have no support for selecting only some columns.
  • No support for GROUP BY.
  • When you want all rows you still have to pass a where clause like '1=1'.
  • etc...

Now you could extend your function to allow for these other possibilities, but it will become more complex and you'll just end up with a custom language for creating SQL queries but doesn't resemble SQL. Sometimes it is a good idea to create a custom language for accessing an SQL database but most often it is just a waste of your time and will make it hard for others to understand your code. Stick to using standard libraries and standard interfaces where possible. If you want to avoid using SQL you could investigate using an Object-Relational Mapping (ORM) instead.

If you want to keep SQL out of your code, try to make an interface that actually does something specific to your application (getFriends, getPostsByUser) instead of something that is too generic.

Mark Byers