What do you do if you see things like this in a script a co-worker wrote:
1.
if ($dbh) {
return 1;
} else {
return 0;
}
2.
$searchsql = sprintf("%s%s",$searchsql,$sql);
3.
while ($stopsearch == 0) {
my $rv = $sth->execute || die $dbh->errstr;
# loads of commented out code
if ($rv eq '0E0') {
$stopsearch = 1;
} else {
# lots more code
$stopsearch = 1;
}
}
In total there is about 400 lines of code. I wrote
a replacement for a similar script that had more features
in about 230. He decided not to use my script and just
rehash his old one. He also doesn't seem to like using
the parsing library I wrote and prefers using about 30
lines of:
($price) = ($input =~ m/(.*)<\/pr>/si); ($saleprice) = ($input =~ m/ (.*)<\/sp>/si); # etc
Maybe you can get him to tap into the community, through this site, perlmonks, various books, etc. Send him links to interesting articles from time to time, until he is reading these sites regularly and learning. He may give more credence when he can see the consensus behind various practices, and he's sure to learn something.
One of Larry's credos is that it's okay to program in Perl baby talk, and we won't laugh. That may be so, but I'm still pretty ashamed of some of the things I produced more than a year or two ago. It's very obvious from a survey of my projects that I have been learning continuously.
return ( $dbh ? 1 : 0 );
Some people, not I, stay away from the ?: like the plague because they think it's unreadable.
There's a trick I've used in C to turn an integer into 1/0:
return !!dbh;
This is too clever, I think, simply:
return ( dbh != 0 );
Is probably preferable.
But, doing this in Perl:
return !!$dbh;
Returns 1 and "".
If you really want to return numeric 1 or 0, the if statement your co-worked had is not terrible, really. Very easy to understand and it works. Returning $dbh, while satisfying the same logical tests to the caller, may be returning any data type.
The other two are, well, harder to justify, to say the least.
Nothing at first. Do they work? Yes. Are they icky? Sure, but they also work. Are they wrong? No, not at all.
Now, I'm not saying you ignore it, but be logical and methodical about it. "Seek first to understand, then to be understood." Try to understand why the code is written that way. For example:
It would be damn easy to change that toif ($dbh) {
return 1;
} else {
return 0;
}
but what are the effects? What if the function relies on 1 or 0? It shouldn't, of course, but that doesn't mean you can go changing the code willy-nilly. If someone's going to return 1 or 0 for true or false, chances are he'll check those values explicitly, too.return defined $dbh;
Also, when you examine why things are, look beyond the obvious. For example:
Sure, that should be$searchsql = sprintf("%s%s",$searchsql,$sql);
or more likely (I'm guessing)$searchsql.= $sql;
but why stop there? Assuming that the 2nd $sql is conditions on the SQL, why is he building SQL by hand? Why isn't he using bind variables? Maybe that's the real improvement to be made.$searchsql.= $where_clauses;
And before you go changing this stuff, do you have tests in place to catch the errors you're going to introduce? There's no such thing as a simple one line change. Don't fool yourself into thinking "I'll just change one line". All it takes is to type
instead of$searchsql = $sql;
to make a simple, one-line change that didn't need to actually be made into a huge blunder on your part that breaks code.$searchsql.= $sql;
What about the human side of things? You say "I've tried to subtly suggest to him and my boss that I could help him with his programming. So far I have been ignored."
First, don't be subtle. If it needs to be changed, then it needs to be changed, and it's your responsibility to make it clear. And if it doesn't actually need to be changed, then don't bother suggesting it.
Second, explain what the benefits are. Explain to your boss the business reasons for helping your co-worker with his code. Does the current codebase hinder maintainability? Is it more prone to errors? Or is it just that you don't like it? If you can't come of with solid reasons to take a course of action, and you can't explain them clearly, then don't fight the fight.
Off the cuff, here's how I would transform this stuff and why.
if ($dbh) {
return 1;
} else {
return 0;
}
That would become:
return 1 if $dbh;
Unless there is a demonstrable need to return zero for false, I consider it a bug. What if the results are assigned to an array? The array then has one element, zero, which causes an if (@array) conditional to incorrectly evaluate as true. Further, since boolean tests don't warn about undefined values, why bother to ever return the zero? Unless that number is a count, it's probably not necessary -- and it's a bug waiting to happen.
$searchsql = sprintf("%s%s",$searchsql,$sql);
Wow. What a waste:
$searchsql
It's much easier to read and some maintenance programmer isn't going to come along and try to figure out if there is something deeper going on. I was sitting here trying to see if there was something deeper going on, but all this is doing is writing obfuscated code.
while ($stopsearch == 0) {
my $rv = $sth->execute || die $dbh->errstr;
# loads of commented out code
if ($rv eq '0E0') {
$stopsearch = 1;
} else {
# lots more code
$stopsearch = 1;
}
}
This becomes something along the lines of:
unless( $stopsearch ) {
my $rv = $sth->execute; # RaiseError should probably have been set in the constructor
if ( '0E0' eq $rv ) {
some_error_routine( $rv, $dbh );
last;
}
# do my stuff
}
That's just roughly thrown together and may not match what you need, but it's easier to read. However, just the way $stopsearch is appearing before the 'execute' makes me a wee bit curious. I wonder if there is a deeper issue here. Also, what are the search parameters? Are they static? If not, I saw that there were no arguments passed to the execute method, so I am wondering if $dbh->quote was called on the arguments earlier? Just a thought
Idiomatic Perl code is easier to read and it's less likely to be buggy. This means money saved in both development and maintenance. If you start writing unit tests, you can possibly show how these things can fail by creating the proper test cases. Anyone who writes code this sloppy is probably creating plenty of bugs. Then, if you can pull that off, rewrite the code correctly and rerun your tests. Thus, you can gain the benefit of testing and teaching idiomatic Perl.
Fair warning: this is politically risky. If you're not very well regarded (and it sounds like that may be a problem if they're not listening to you), then this could get you branded a trouble maker.
Or you could take a route I took a couple of years ago. The vice president of my company was constantly moving crud into production and I would silently rewrite it so that it worked. He never noticed