Missing the big picture

chromatic is a bit harsh on Seda Özses’s article “Very simple login using Perl, jQuery, Ajax, JSON and MySQL” on IBM developerWorks, and his tone encouraged a lot of other people to be equally harsh. This has always been one of the biggest problems for technical communities: they forget that a person is on the other side. Not only that, they forget that a person is also on their side. I think chromatic could have modulated his tone and formed a very useful post had he actually explained his changes in the code.

Now, to be fair, the original article, which I did not see, was worse than the current one according to the gossip. And, I mean gossip. What is this? Middle school? I saw that Dave Cross tweeted about the bad article, then whoever controls The Perl Foundation twitter account retweeted it with five other people. If anyone wrote to Seda first with a patch, please speak up, but if your tweet isn’t “I wrote to so and so with a patch and they told me to get stuffed”, Twitter isn’t always the best route. Sure, Kevin Smith may have Southwest sucking up to him now, but if you followed the whole story, it was only for him.

chromatic gracefully slides over the fact that Seda actually updated the article because that belies his intent of public shaming. He’s still on the attack because no bad code can be allowed to live. And, to be fair to chromatic, he says “It’s okay to write baby Perl, but it’s baffingly irresponsible to publish said baby Perl as an example from which you expect other novices to learn.” I say “It’s lame to tell someone their fly is down by screaming it to the entire room”.

Here’s the code that Seda Özses presented when chromatic started his post (and is not the original or current code in the article):

#!/usr/bin/perl
use CGI qw(:all);
use DBI;
use DBD::mysql;

# read the CGI params
$CGI = new CGI();
$username = $CGI->param("username");
$password = $CGI->param("password");

# connect to the database
$DBH = DBI->connect("DBI:mysql:database=mydb;host=localhost;port=2009",
	"mydbusername", "mydbpassword")
	or die($DBI::errstr);

# check the username and password in the database
$query = qq{SELECT id FROM users WHERE username=? && password=?};
$query = $DBH->prepare($query);
$query->execute($username,$password);
$ref = $query->fetchrow_hashref() || 0;
$userID = $ref->{"id"};

# create a JSON string according to the database result
$JSON = ($userID) ?
	qq{{"success" : "login is successful", "userid" : "$userID"}} 	:
	qq{{"error" : "username or password is wrong"}};

# return JSON string
print qq{Content-type: application/json; charset=utf8\n\n};
print $JSON;

What’s chromatic’s beef here? He doesn’t say because he falls into the trap that many über-coders do: they think that code speaks for itself. It doesn’t, which is why I contend that so many people can’t read your code. chromatic doesn’t care as much about teaching anyone to do better as making his point, so he becomes the guy that makes you think all Perlers are assholes.

I’ll explain some of it though:

  • Seda doesn’t use strict or warnings. These are handy development aids to keep you on track. I don’t use warnings in production code either, though, since I can easily turn them on while I develop. Debate among yourself on that one, though.
  • Seda imports :all from CGI.pm, but doesn’t use anything that she imported. So, there’s a little cut-n-paste cargo-culting there. It’s not that big of a deal though.
  • Seda loads the DBD::mysql, although DBI will do that for her. You only need to load the modules you directly use. You don’t have to directly load the modules that other modules use.
  • Seda doesn’t validate the user input.
  • $ref might not be a reference, so the $ref->{"id"} might blow up.
  • Seda doesn’t use taint-checking, a knee-jerk prophylactic that’s a favorite among people who just want to be snide.
  • Seda uses the indirect object form, new CGI. This was the documented style up through CGI 3.45, released in August 2009. Perl 5.10.1 came with CGI 3.43. That documented style didn’t change until Perl 5.12.0 included CGI 3.48.
  • chromatic asserts that you can’t make a simple CGI response without using the CGI.pm module.
  • chromatic doesn’t like the style.

And here’s chromatic’s code, which I think is equally bad:

#!/usr/bin/perl -T

use strict;
use warnings;

use CGI;
use DBI;

my $q        = CGI->new;
my $username = $q->param("username");
my $password = $q->param("password");

my $dbh = DBI->connect(
	'DBI:mysql:database=mydb;host=localhost;port=2009',
	'mydbusername', 'mydbpassword'
) or die $DBI::errstr;

my $sth = $dbh->prepare(
	'SELECT id FROM users WHERE username=? and password=?;'
);
$sth->execute($username, $password);

my $json;

if (my ($id) = $sth->fetchrow_array)
{
	$json = qq|{"success" : "login is successful", "userid" : "$id"}|;
}
else
{
	$json = qq|{"error" : "username or password is wrong"}|;
}

print $q->header(
	-type    => 'application/json',
	-charset =>  'utf-8',
), $json;

So what’s wrong with chromatic’s code? First, because of his position, he’s set himself up as a bigger target. He can’t complain about someone else’s code then make a bunch of mistakes himself.

  • chromatic has turned on taint-checking, but then does nothing with it. He doesn’t scrub the PATH environment variable, untaint the CGI input, or use the taint features of DBI. He doesn’t do most of the stuff I go through in my security chapter of Mastering Perl.
  • chromatic still uses CGI.pm. People do that because it’s in the Standard Library. It’s an ugly, monolithic waste of cycles and memory in this case though. Something like CGI::Simple would do the job nicely without the bloat.
  • While we’re both being pedantic, chromatic leaves hard-coded configuration inside the script. He should get the username, password, and non-standard port from non-code sources, such as a configuration file.
  • Instead of returning a valid response on a failure to connect to the database, chromatic just die‘s. The user will just see some sort of “500 Internal Server Error” page. Really? In 2011, from the author of Modern Perl?
  • chromatic uses the DBI error in his die message, potentially exposing infrastructure details that an attacker might use later. The user doesn’t need to know that level of error. And, DBI already has the RaiseError feature to do this. You want the error message to go into the log file and not make it out to the user. Where is that STDERR really going?
  • chromatic turns on strict and warnings. Although that adds no value to this code, if someone develops it further, those pragmas can help can help.
  • chromatic loads only the modules he directly uses, and doesn’t import anything.
  • chromatic violates the DRY principle. How many times does he have to type $json to assign to it once? Seda’s version was better, even though she needed to improve her conditional. I would have used do, myself.
  • chromatic insists that she use the CGI.pm module to make a CGI response, but he lets himself make a JSON data structure by hand. What’s in $id? chromatic didn’t encode that for JSON, so he can output an invalid object there. Hard-coding message formats is also quite a bad practice, but chromatic lets that slide too. Wrap that in an interface that all of your applications can use.

Again, to be fair, chromatic says in a comment “I wanted to make the minimal responsible changes to the program to improve its modernity. I also prefer to have connection information outside of my programs, but that’s also a larger change.” I don’t think he accomplished that, but he doesn’t let himself because he focuses on the least useful parts of the whole mess.

But, there’s no real improvement here. It’s the same code with the same problems, structurally. chromatic’s real beef is that someone without a lot of Perl experience used baby Perl in an article. That is, unless you are an expert, you shouldn’t be writing about Perl, at least in his mind. There’s no place for compassion or humanity: you either get it exactly right the first time, or he will publicly shame you, although passive-aggressively because he won’t use your name (it doesn’t matter because you aren’t a real person anyway).

It’s amazingly difficult to present useful code in a tutorial, and chromatic knows that. It’s a side issue to him though. If the Perl authors all wrote complete and correct programs, with good style, good error checking and handling, proper configuration, proper logging, and everything else a robust program needs, no matter the language, we wouldn’t be able to illustrate the concept that we want to point out.

I’m not defending the code that Seda posts, but I understand the situation. chromatic understands it to. An author’s job is to show enough to illustrate the point without causing unnecessary distractions. The reader’s job, at least in the mind of authors, is to learn the key points and integrate that with other knowledge. Tutorials don’t present production-ready programs just for that reason. Authors expect (and respect) that their readers aren’t idiots, even if most of them are. Would you read any article that went through every special and edge case just to show you how to output some JSON?

And, to be honest, if I were starting out in Perl today and ran into people like chromatic, I probably would find some other language to use. I wrote a lot of dumb Perl. It showed up in The Perl Journal and at The Perl Conference. I still remember Tom Phoenix coming up to me, when I knew no one else in the community, and saying “That’s really neat, but what if you did this”. I met a lot of welcoming and nice people, and I decided I want to keep meeting them so I convinced a lot of people to form NY.pm, then Perl Mongers.

Leave a comment

0 Comments.

Leave a Reply

You must be logged in to post a comment.