But no-one pointed out that if you're using DBI, you should not be constructing SQL with arbitrary values like that. You should be using placeholders. What if someone hands you a value with don't in it?

-- Randal L. Schwartz, Perl hacker

Excellent point, o-sensei merlyn. Error/ value checking will occur at a different place in the script -- no unsafe value should reach this sub.
update
for those of you wondering, an apostrophe will stun most SQL parsers. The short answer is something like

s/'/''/g
[download]

Or, a short answer that works is something more like:

$quoted = $dbh->quote($raw);
[download]

But it's much better to use the placeholders, as they can be trained in the SQL datatypes so as to quote correctly for those DBDs that have specific needs.

-- Randal L. Schwartz, Perl hacker

my($l_tablename, %l_data) = @_;
my(@fields, @values);
push(@fields, $_) && push(@values, $1_data{$_})
    for(keys %1_data);

my($1_prepare);
{
    local $" = ','; # eliminate join statements
    $1_prepare = qq(
        INSERT INTO $1_tablename (@fields) 
        VALUES (@values)
    );
}
[download]

1. The first literal character in a variable can't be a numerical value. So "1_" will have to go! =)
2. In your original code, you assign a hash to a shift statement. This won't work because you'll only get the first value from the list you pass. The assignment arrow operator => acts the same as a comma in a list, and lists flatten. So something like this will happen:
   

   @list = ('arg1', ('field1' => 'value1', 'field2' => 'value2'));
   # above same as:
   @list = ('arg1', 'field1', 'value1', 'field2', 'value2');
   
   $table = shift(@list);  # 'arg1'
   %fields = shift(@list); # 'field1'
   [download]

   The last statement, in order to put all the remaining values from @list into %fields should read: %fields = @list;
3. Writing a function to prepare generic insert statements is generally a bad idea. This is just asking for people to call your function with invalid table names, field names, and data types. If the database itself changes, it becomes more difficult to fix the code because you have to track down every place where this subroutine is being called.

There's probably a better way to do what you're trying to do. Writing an insert subroutine for each table in the database that allows new records might be a better solution. If you are really ambitious, you can even write an OO wrapper for each table, with select, insert, update, and delete methods. Then you can write things like:

    my $person = new MyDB::Person(4);
    $person->update(name => 'John B.');
[download]

...or whatever. Just a few thoughts for you to mull over. {g}

Programming Perl from Larry, Tom, and our very own Randal published by O'Reilly under the entry for values (pg. 239) mentions "...it is the same order as either the keys or each function would produce...".

Hope this helps.
=Blue
...you might be eaten by a grue...

I note that the second line of your sub is: my %l_fields = shift; Is that a transcription error? As shown, it will create a hash with one element. You don't show how the subroutine is called, but that line should be either: my %l_fields = @_; or my %l_fields = %{ +shift };

Is that a transcription error? As shown, it will create a hash with one element.

The line is actually my ($l_tablename,%l_fields)  = @_;. I don't know what came over me to write that. I blame my general lack of caffeination.

# values %foo
map {$foo{$_}} keys %foo
[download]

sub update_table{
    my $l_tablename = shift;
    my %l_fields = shift;
    my $l_fieldnames;
    my $l_fieldvalues;
    map {
        $l_fieldnames .= $_;
        $l_fieldvalues .= $l_fields{$_};
    } keys %l_fields;

    my $l_prepare = "INSERT $l_tablename (" . $l_fieldnames . ")\n VAL
+UES(" . $l_fieldvalues . ")";
    return $l_prepare;
}
[download]