ansaurus

Question

Improving this longwinded and repetative sql script..

Answer 1

+1 A:

While your alignment of column names and perhaps a hidden form field for table name would be quicker to code and more elegant to code against you would be opening yourself up to someone spoofing an HTML file to control the inserts to a table! Consider refactoring in other ways to optimize your library. Keep thinking / you're bound to come up with a better solution within the context of your problem domain.

Tahbaza 2010-08-06 01:38:30

thanks for that Tahbaza.

KeenLearner 2010-08-06 03:46:39

Answer 2

+1 A:

Usually, one has some base models to do this, for example (don't be to harsh about it's failtings, I just want to give a short example):

class Speaker {
    function __construct($data){
         foreach($data as $key => $value) $this->$key = $value;
    }
    function save(){
        $query = 
            ($this->speaker_id ? 'UPDATE ':'INSERT INTO').' speakers '
            ."SET speaker_name='%s',
              speaker_content='%s',
              speaker_status='%s'"
            .($this->speaker_id ? ' WHERE speaker_id=%d ':'');
        mysql_query(sprintf($query(
             mysql_real_escape_string($this->speaker_name),
             mysql_real_escape_string($this->speaker_content),
             mysql_real_escape_string($this->speaker_status),
             intval($this->speaker_id)));
        if(!$this->speaker_id) $this->speaker_id = mysql_insert_id();
    }
}

And then:

switch($_POST['process']) {
   case 'speaker':
       $speaker = new Speaker($_POST);
       $speaker->save();
       header('Location: /speakers?speaker_id='.$speaker->speaker_id);

But for new projects, I would suggest a more complete MVC pattern (where Event & Speaker can be subclasses from some base databasemodel, possibly able to generate some form based on settings), and use prepared statements, they will make your sql queries easier & safer. never use addslashes, for good ol' mysql_query use mysql_real_escape_string

Wrikken 2010-08-06 01:44:34

Wrikken 2010-08-06 01:49:15

and have all these models repeated as well lol :)

Col. Shrapnel 2010-08-06 01:57:07

Wrikken 2010-08-06 02:21:32

and have even more code as at the end...

Col. Shrapnel 2010-08-06 02:26:15

Not necessarily. As long as you don't need to support edge cases, why bother with those. And it would be DRY, as far as the question is concerned :P

Wrikken 2010-08-06 02:35:24

Col. Shrapnel 2010-08-06 02:51:13

Thanks heaps wrikken.

KeenLearner 2010-08-06 03:48:01

Wrikken 2010-08-06 06:08:46

Answer 3

+1 A:

1) some flaws in your concept

every piece of data you're going to put into quotes in your query, should be processed with mysql_real_escape_string, as you cannot know what can be inside.
never use a table name passed from the client side. there can be malicious code instead of mere table name as well.
same for the field names. every identifier should be hardcoded in your script.

2) as for the DRY - it's simple. just note similar parts in your code and put them into function. only fields differ? okay, make a function that take fields list and produce an SQL statement of them.

Luckily, Mysql let us use similar syntax for both insert and update. So, a very simple function like this one can help:

function dbSet($fields) {
  $set='';
  foreach ($fields as $field) {
    if (isset($_POST[$field])) {
      $set.="`$field`='".mysql_real_escape_string($_POST[$field])."', ";
    }
  }
  return substr($set, 0, -2); 
}

So, you can make your code shorter:

case 'speaker':

  $table = "speakers";
  $fields = explode(" ","speaker_name speaker_content speaker_status");

  if(isset($_POST['speaker_id'])) {
    $id = intval($_POST['speaker_id']);
    $query  = "UPDATE $table SET ".dbSet($fields)." WHERE id=$id";
  } else {
    $query  = "INSERT INTO $table SET ".dbSet($fields);
  }
  mysql_query($sql) or trigger_error(mysql_error().$query);  
  if($_POST['speaker_id']) $id = mysql_insert_id();
  header('Location: speakers?speaker_id='.$id);        

break;

if all your actions are such alike, you can make more high leveled functions:

case 'speaker':

  $table = "speakers";
  $fields = explode(" ","speaker_name speaker_content speaker_status");

  if(isset($_POST['speaker_id'])) {
    $id = intval($_POST['speaker_id']);
    dbUpdate($table,$fields,$id);
  } else {
    $id = dbInsert($table,$fields);
  }
  header('Location: speakers?speaker_id='.$id);        
  exit;

break;

and even more high level

case 'speaker':

  $table = "speakers";
  $fields = explode(" ","speaker_name speaker_content speaker_status");
  $id = dbMagic();
  header('Location: speakers?speaker_id='.$id);        
  exit;

break;

But I won't go into that. I'd stop at 1st option, because it's pretty straightforward and there are always some little things not fit into such a broad concept.

Col. Shrapnel 2010-08-06 02:20:47

A lot of pointers and tips there. I'm studying this getting my head around it.

KeenLearner 2010-08-06 03:47:09

@Keen yeah and even more than it seems :) feel free to ask

Col. Shrapnel 2010-08-06 04:08:07

Cheers Col. I am aware of the security issue in having transparent db table names and columns - but I figure since this is a private backend nobody but the system users will access, with each page session logged and protected that it wouldn't pose an issue? I would not do something like this on a open website.With your solution, I'm still having to break the page into cases which I'm trying to avoid. I guess not being a ninja at code I have to make a sacrifice :)

KeenLearner 2010-08-06 06:58:12

ansaurus

tags:

views:

answers:

Improving this longwinded and repetative sql script..

related questions