Re: EXAMPLE -- Re: Strange issue with `CHOMP' not working...

On 11 Apr 2006 10:56:41 -0700, Paul Lalli <mritty@xxxxxxxxx> wrote:
Ignoramus10782 wrote:
I rewrote my module to make it slightly nicer.

Not sure if anyone is interested, but here it is:
==================================================

package Algebra::Config;

use strict;


Why aren't you using warnings?

No good reason, I guess. I added 'use warnings' to the module. It did
not change anything, but I agree that it is a good idea.

require Exporter;

use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS);

Are you specifically writing this module to be compatable with perl
5.005? May I ask why? If not, you should be enabling warnings and
using the 'our' keyword instead of the vars pragma

Hm, I am open minded, but why should I do that? What is the reason?


@ISA = qw(Exporter);
@EXPORT= qw( get_config_var get_data_dir get_algebra_root );
@EXPORT_OK = qw( get_config_var );

$VERSION = 2000.0426;

use vars qw( $algebra_data_dir $_values );

Why are these global variables? Are you planning on accessing them
outside of this module? They should probably be lexicals - especially
one that starts with an underscore, which traditionally refers to a
"private" variable.

I use them to cache data to speed up access. The data is static and I
do not want it to be computed every time.


sub get_algebra_root {
my $algebra_root = $ENV{'ALGEBRA_ROOT'} || $ENV{'DOCUMENT_ROOT'}
|| die "Webmaster has to define ALGEBRA_ROOT environment variable!";

Seems to me like this could be better as a 'croak' statement than a
die. Why do you care what line in get_algebra_root this happened?

I agree. I changed that line.

return $algebra_root;
}

sub get_config_var {
my ($category, $variable) = @_;

return $_values->{$category}->{$variable} if exists $_values->{$category}->{$variable};

my $algebra_root = get_algebra_root;
my $config_file = "$algebra_root/etc/$category";

open( CONFIG, $config_file ) or die "Config file '$config_file' does not exist or is not readable.";

1) Use lexical variables instead of global barewords (again, unless
you're specifically developing for Perl 5.005 or earlier)
2) Use the three-argument form of open
3) Those are not the only two possible reasons the open could fail. Do
not guess - let Perl tell you specifically why, by including the $!
variable in the die() statement.

open my $CONFIG, '<', $config_file or die "Could not open
'$config_file': $!";

I used the $! and decided to skip the lexical suggestion.Thanks.

while ( $_ = <CONFIG> ) {

Why are you explicitly assigning to $_? Nothing "wrong" with this, I
guess, it just strikes me as odd.

I agree, I changed it.

It would look more normal to assign to a lexical variable, or to let
the while(<>) magic happen automatically.

chomp;

So for all your insistance that localizing $/ was the best suggestion
you got, you still completely ignored it? Why?

Everything is fine now and these problems no longer occur. However, I
agree that I should print the $/ should the problem ever reoccur, so I
changed my script top print $/.


if ( /(\n|\r)/ ) {
my $str = substr( $_, 0, 80 );
print STDERR "Algebra::Config: BAD BAD BAD Input string after chomp but BEFORE regex: '$str'.\n";

'print STDERR' is generally better written 'warn', but in this case is
probably better written 'carp', so you know where the input came from.

Instead of carp, I simply print the filename.

}

s/(\n|\r)//g;

Seems silly to have to do this match twice. Why not simply make this
the condition of your test above?

I wanted to print the string before I clean it.


next if( /^\s*#/ );
next if( /^\s*$/ );
my( $key, $value ) = split( /\s*=\s*/, $_ );

$_values->{$category}->{$key} = $value;
}

close( CONFIG );

return $_values->{$category}->{$variable} if exists $_values->{$category}->{$variable};

$_values->{$category}->{$variable} = undef;

return undef;
}


sub get_data_dir {
my ($category, $variable) = @_;

my $dir = get_config_var( $category, $variable );
if ( $dir !~ /^\// ) {

Yuck. I hate leaning toothpicks.

if ($dir !~ m{^/}) {

Of course, I also generally dislike regexps when a substr() will do,
so:

if (substr($dir, 0, 1) ne '/') {

I think that I like what I have, better.

$dir = $algebra_data_dir . "/$dir";

Why are you using both explicit concatenation and interpolation in the
same statement? Seems inconsistant.

Agreed. I changed it.

package Algebra::Config;

use strict;
use warnings;

use Carp qw/croak confess/;

require Exporter;

use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS);

@ISA = qw(Exporter);
@EXPORT= qw( get_config_var get_data_dir get_algebra_root );
@EXPORT_OK = qw( get_config_var );

$VERSION = 2000.0426;

use vars qw( $algebra_data_dir $_values );

sub get_algebra_root {
my $algebra_root = $ENV{'ALGEBRA_ROOT'} || $ENV{'DOCUMENT_ROOT'}
|| croak "Webmaster has to define ALGEBRA_ROOT environment variable!";
return $algebra_root;
}

sub get_config_var {
my ($category, $variable) = @_;

return $_values->{$category}->{$variable} if exists $_values->{$category}->{$variable};

my $algebra_root = get_algebra_root;
my $config_file = "$algebra_root/etc/$category";

open( CONFIG, $config_file ) or die "Config file '$config_file' does not exist or is not readable ($!).";
while ( <CONFIG> ) {
chomp;

if ( /(\n|\r)/ ) {
my $str = substr( $_, 0, 80 );
print STDERR "Algebra::Config: BAD BAD BAD Input string after chomp but BEFORE regex: '$str' (\$/ = '$/'), file '$config_file'.\n";
s/(\n|\r)//g;
}


next if( /^\s*#/ );
next if( /^\s*$/ );
my( $key, $value ) = split( /\s*=\s*/, $_ );

$_values->{$category}->{$key} = $value;
}

close( CONFIG );

return $_values->{$category}->{$variable} if exists $_values->{$category}->{$variable};

$_values->{$category}->{$variable} = undef;

return undef;
}


sub get_data_dir {
my ($category, $variable) = @_;

my $dir = get_config_var( $category, $variable );
if ( $dir !~ /^\// ) {
$dir = "$algebra_data_dir/$dir";
}
return $dir;
}

$algebra_data_dir = get_config_var( "general", "algebra_data_dir" ) || $ENV{ALGEBRA_ROOT};

$_values = {};

1;

.

Relevant Pages