Storing '1' in a Set-hash Considered Harmful
I know using Considered Harmful is considered harmful, but this is bad; almost as bad as unrestrained goto
usage.
Ok, I've harped on this in a few comments, but I think I need a full response to demonstrate the issue.
Lets say we have a daemon process that provides back-end inventory control for a shop that sells widgets.
my @items = qw(
widget
thingy
whozit
whatsit
);
my @items_in_stock = qw(
widget
thingy
);
my %in_stock;
my @in_stock(@items_in_stock) = (1) x @items_in_stock; #initialize all keys to 1
sub Process_Request {
my $request = shift;
if( $request eq REORDER ) {
Reorder_Items(\@items, \%in_stock);
}
else {
Error_Response( ILLEGAL_REQUEST );
}
}
sub Reorder_Items{
my $items = shift;
my $in_stock = shift;
# Order items we do not have in-stock.
for my $item ( @$items ) {
Reorder_Item( $item )
if not exists $in_stock->{$item};
}
}
The tool is great, it automatically keeps items in stock. Very nice. Now, the boss asks for automatically generated catalogs of in-stock items. So we modify Process_Request()
and add catalog generation.
sub Process_Request {
my $request = shift;
if( $request eq REORDER ) {
Reorder_Items(\@items, \%in_stock);
}
if( $request eq CATALOG ) {
Build_Catalog(\@items, \%in_stock);
}
else {
Error_Response( ILLEGAL_REQUEST );
}
}
sub Build_Catalog {
my $items = shift;
my $in_stock = shift;
my $catalog_response = '';
foreach my $item ( @$items ) {
$catalog_response .= Catalog_Item($item)
if $in_stock->{$item};
}
return $catalog_response;
}
In testing Build_Catalog() works fine. Hooray, we go live with the app.
Oops. For some reason nothing is being ordered, the company runs out of stock of everything.
The Build_Catalog()
routine adds keys to the %in_stock
, so Reorder_Items()
now sees everything as in stock and never makes an order.
Using Hash::Util's lock_hash
can help prevent accidental hash modification. If we locked the %in_stock
before calling 'Build_Catalog()` we would have gotten a fatal error and would never have gone live with the bug.
In summary, it is best to test existence of keys rather than truth of your set-hash values. If you are using existence as a signifier, don't set your values to '1' because that will mask bugs and make them harder to track. Using lock_hash
can help catch these problems.
If you must check for the truth of the values, do so in every case.