Beefy Boxes and Bandwidth Generously Provided by pair Networks
laziness, impatience, and hubris

DBI Style Inquiry

by edict (Novice)
on Jun 27, 2013 at 08:28 UTC ( #1040954=perlquestion: print w/replies, xml ) Need Help??
edict has asked for the wisdom of the Perl Monks concerning the following question:

Hello monks,

I have been told that I'm sorta OCD when it comes to my programming style, and being relatively new to Perl, I am seeking wisdom.

My code needs to be easily readable and uniformly.. um.. formatted, and concise. So let me get to the point.

Here's some code to grab info from a database (I only need one field). It looks terrible to me, but it works; and I am asking advice on how to make it less fugly.

sub get_loc { chomp $_[0]; my $dbh = DBI->connect("DBI:mysql:tracker:", 'myuser', +'mypassword' ) or die "Connect to database failed: $DBI::errstr\n"; my $sth = $dbh->prepare("SELECT location FROM servers WHERE name=? + LIMIT 1") or die "Couldn't prepare statement: $DBI::errstr\n"; $sth->execute($_[0]); return my @loc = $sth->fetchrow_array(); $sth->finish; $dbh->disconnect or die "Error disconnecting: $DBI::errstr\n"; }

Thank you!

Replies are listed 'Best First'.
Re: DBI Style Inquiry
by muba (Priest) on Jun 27, 2013 at 08:48 UTC

    Fugly is subjective, of course. However, what do you think about this?

    sub get_loc { chomp $_[0]; # Aligned the "or" clauses with the main expressions my $dbh = DBI->connect("DBI:mysql:tracker:", 'myuser', 'mypas +sword') or die "Connect to database failed: $DBI::errstr\n"; my $sth = $dbh->prepare("SELECT location FROM servers WHERE name=? LIMIT + 1") or die "Couldn't prepare statement: $DBI::errstr\n"; $sth->execute($_[0]); # No point in assigning if you aren't going to use @loc anyway # return my @loc = $sth->fetchrow_array(); return $sth->fetchrow_array(); # This is dead code. Execution will jump out of the sub due to the + return statement above! # Put disconnect call and "or" clause on same line $sth->finish; $dbh->disconnect or die "Error disconnecting: $DBI::errstr\n"; # Aligned "}"-line with "sub get_loc {" line }
      Awesome, I appreciate you giving time to formatting as well as pointing out the superfluous code!
Re: DBI Style Inquiry
by mje (Curate) on Jun 27, 2013 at 08:49 UTC

    How about (untested)

    sub get_loc { chomp $_[0]; my $dbh = DBI->connect("DBI:mysql:tracker:", 'myuser', 'mypassword', {RaiseError => 1}); my $a = $dbh->selectall_arrayref(q/SELECT location FROM servers WH +ERE name=? LIMIT 1/, undef, $_[0]); $dbh->disconnect; return $a->[0][0]; }

    I've no idea why in your example you'd return before disconnecting as even though your $dbh goes out of scope you had error checking on the disconnect. RaiseOnError causes a die on error avoiding all those "or die". finish is not required if you fetch all off the result-set. selectall_arrayref is less code than prepare/execute.

    Do you really want to connect and disconnect each time? If you were doing this alot you could prepare the query once and pass the sth to selectall_arrayref.

      Wow, that is much cleaner and clear than what I had going. Thanks for pointing out the flaws. I still have much to learn and it's awesome having access to a community so willing to assist.
Re: DBI Style Inquiry
by Tux (Abbot) on Jun 27, 2013 at 10:15 UTC

    I agree with using consistent style is important, and in perl it is relatively easy to get to a consistent style using perltidy

    Carve your preferences in ~/.perltidyrc and use whatever fits best to have the perltidy command do its job.

    THINK about every setting. You do not have to agree with my choices, but if you take the time to read them, you will definitely agree AND disagree on several points.

    I fully support mje's suggestion to connect with default attributes, like

    my $dbh = DBI->connect ($dsn, $user, $pass, { RaiseError => 1, PrintError => 1, ChopBlanks => 1, ShowErrorStatement => 1, FetchHashKeyName => "NAME_lc", });

    Enjoy, Have FUN! H.Merijn
Re: DBI Style Inquiry
by Eily (Prior) on Jun 27, 2013 at 08:51 UTC

    You could "name" your first parameter by assigning its value to a lexical. Like chomp(my $param = $_[0]);. But that's not really much change, and that's up to you.

    However, there is a problem with your code. The "return" statement exits the sub immediatly, and everything below that point is skipped. Besides, you assign the result of fetchrow_array() to an array that you never use (you could have written return $sth->fetchrow_array();). If you want the end of the sub to be run, you should have something like:

    sub get_loc { ... my @loc = $sth->fetchrow_array(); $sth->finish; $dbh->disconnect or die "Error disconnecting: $DBI::errstr\n"; return @loc; }

      Concerning the return statement, apparently it is commonly known that the subroutines exit there but I was ignorant about that fact (I've never taken a programming class, to be honest). Thank you for your input!
Re: DBI Style Inquiry
by kcott (Chancellor) on Jun 27, 2013 at 08:52 UTC

    G'day edict,

    Welcome to the monastery.

    Take a look at the Perl style guide for some general guidelines on formatting your code.

    With the specific code you posted, you're returning from the subroutine before the last two statements are run. You probably want something closer to this:

    sub get_loc { ... my @loc = ... $sth->finish; $dbh->disconnect ... return @loc; }

    -- Ken

      I appreciate the link, I'll check it out, thanks!
Re: DBI Style Inquiry
by clueless newbie (Chaplain) on Jun 27, 2013 at 11:41 UTC

    To quote "Perl Best Practices" - "Subroutines always receive their arguments in the @_ array. But accessing them via $_[0],$_1, etc. directly is almost always a Very Bad Idea." See page 178

    Conway goes on to say "Moreover, it's easy to forget that each element of @_ is an alias for the original argument; that changing $_[0] changes the variable containing that argument."

    So please unpack "@_" and "chomp" the copy. Who knows the calling code might need the trailing "$/"!

      It would behoove the OP to get this book and use it hand in hand with Perl::Critic; In combination with Perl::Tidy, he may keep his OCD in check. Once he forms his own ways and opinions, he can start to customize both.
Re: DBI Style Inquiry
by Ralesk (Pilgrim) on Jun 27, 2013 at 11:50 UTC

    Generally, I do a my ($this, $that, $foo, $bar) = @_; at the start of my subs, rather than to access the @_ array directly. In case we’re talking about a method — where Perl will put the object as the first argument — it would go like this:

    sub foo_method { my $self = shift; my ($this_arg, $that_arg, $whatever) = @_; ...; }

    Another thing about DBI, I almost never use fetchrow_array anymore, but much more commonly use fetchrow_hashref which will allow you to refer to the returned columns by their names. Make sure to alias the columns with the SQL AS column_name phrase though.

    Others have touched some other issues already, so I won’t repeat those.

      But fetchrow_hashref is much slower than the other fetch methods. It doesn't really matter if you fetch just a single row, but when traversing thousands of records, you will notice. For speed use bind_columns (the last example of that section shows how to get the best of both worlds). Here are some (older) speed compares for different database types:

      Enjoy, Have FUN! H.Merijn

        Nice comparison. Would have probably been more interesting to see a graph that isn’t anchored on hashref = 100% though.

        I am surprised by the difference (even if it's small) between fetch and fetchrow_arrayref (your BC and DBC), as DBI docs say:

        "fetchrow_arrayref" $ary_ref = $sth->fetchrow_arrayref; $ary_ref = $sth->fetch; # alias

        Or is the essential difference something else?

        (Also, it would be interesting to have the DBMS version-numbers on those pages; it does say 2006, that's a long time ago. I would say that (arguably, no doubt) PostgreSQL has advanced more than the others in performance in this particular period)

Re: DBI Style Inquiry
by fullermd (Priest) on Jun 28, 2013 at 02:31 UTC

    I must say that I for one find the idea of a function erecting a DB connection to run one query and then tearing it down before returning, to be pretty durn fugly.

    Of course, that can come down to a matter of just what your program needs to do. Maybe it is the best choice in your case. If that's the only place in the whole codebase that does any DB work, and can only ever be called once in an entire invocation, maybe it's a good method.

    But, I don't think that's most of the world. And the Law of Feeping Creatures says it won't be your world long, even if it is at the moment :)

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://1040954]
Front-paged by Corion
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others meditating upon the Monastery: (7)
As of 2018-11-17 16:52 GMT
Find Nodes?
    Voting Booth?
    My code is most likely broken because:

    Results (205 votes). Check out past polls.