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:12.34.56.78", '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
}
[download]

Awesome, I appreciate you giving time to formatting as well as pointing out the superfluous code!

How about (untested)

sub get_loc {
    chomp $_[0];
    my $dbh = DBI->connect("DBI:mysql:tracker:12.34.56.78",
                           '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];
}
[download]

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.

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",
    });
[download]

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;
}
[download]

I appreciate the link, I'll check it out, thanks!

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;
}
[download]

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!

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.

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) = @_;
  
  ...;
}
[download]

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:

• PostgreSQL
• MySQL
• Oracle
• Unify
• Overview of all

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
[download]

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)

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 :)