There are a couple of additional things you might do to improve the readability and maintainability of your code.
One Perlism that comes in hugely handy when you're generating HTML is the alternate string-quoting form. For example:my $html = qq{<font face="Arial, Helvetica, sans-serif" size="2" color="red">};
Note that you no longer have to escape the doublequotes, because qq{} lets you define alternate delimiters. I chose curly braces in this case because they're rare in HTML and minimize the amount of escaping you have to do, but other paired parenthetical characters work too, as do arbitrary non-parenthetical characters such as these:my $html = qq*<font face="Arial, Helvetica, sans-serif" size="2" color="red">*;
(though I must admit I find that using asterisks as a delimiter is not so easy to read).
qq{}, like double quotes, performs variable interpolation; q{}, like single quotes, does not. There's also qx{}, which is equivalent to backticks, and qr{} for constructing regexes. See the section "Quote and Quote-Like Operators" in perldoc perlop for more info.
Secondly, you might want to factor out the HTML formatting, which seems to be the same for every message, into a subroutine or a constant. For example: sub _format_error {
return qq{<font face="Arial, Helvetica, sans-serif" size="2" color="
+red">$_[0]</font>};
}
my $login_blocked = format_error('Login Blocked please wait 3 minutes
+and try again');
# ...etc...
That way you only have to change things in one place if you someday want to alter the HTML formatting. It also makes the list of messages clearer to someone reading your code because they don't have to figure out what's the actual text and what's part of the HTML formatting.Good luck!
$perlmonks{seattlejohn} = 'John Clyman'; | [reply]
[d/l]
[select] |
An alternate possibility would be to use the magic of the AUTOLOAD subroutine, to access data. This would allow for greater simplicity, when it comes to adding methods.
our $AUTOLOAD;
sub AUTOLOAD {
my $self = shift;
ref($self) or return; # Non-OO/Not an autoloaded method..?
return if ( $AUTOLOAD =~ /DESTROY/ ); # don't mess with garbage
+collection
( my $method = $AUTOLOAD ) =~ s{.*::}{};
my ( $code, $name, $ATTRname );
if ( $method =~ /get_(\w+)/ ) {
my $ATTRname = lc($1);
# Could possibly do a check to make sure we have one of a group o
+f specific
# attributes with.
#
# if ( $ATTRname =~ /^(?:html_error_string|error_type|password_va
+lid|user_id
# |other|stuff|you|want|to|access)$/x
# ) {}
no strict qw{refs};
# create and register the method
*{$AUTOLOAD} = sub {
my $self = shift;
return $self->{$ATTRname};
};
unshift @_, $self;
goto &{$AUTOLOAD};
}
return;
}
Gyan Kapur
gyan.kapur@rhhllp.com
| [reply]
[d/l] |
| [reply] |
Nice approach you're taking.
Since your class seems to represent an error, you might want to use Error as a base class.
Hope this helps, -gjb-
| [reply] |
This seems more like a way of presenting an error to the user than an error itself; that is, I don't really see a potential is-a relationship to Error. Instead, I'd use this code as a way to translate an Error object into user-readable form.
Since there's obviously a lot of code missing, I have to speculate here, but it looks to me like the module is responsible for both controlling login behavior and displaying errors to the users. More ideally, these two pieces of functionality would be decoupled, preferably into separate modules but you could get away with keeping them separate within one. To use Error, I'd envision that the part of the module that does the real work of login validation (i.e., sanity checking the password, comparing it to an encrypted version in the DB, etc.) would raise an exception appropriate to the error condition encountered. The exception would contain the error string to be returned to the user. For example, you might have an exception hierarchy like so:
Error
| # arrow indicates an inheritance relation.
V
LoginError
| |
V V
LoginBlocked PasswordError
| \
V V
PasswordBlank PasswordInvalid
... and so on.
And you'd indicate that an error has occurred with code like throw PasswordInvalid("PasswordNotFound\n") unless ($some_test_here);
Your calling code, presumably a CGI, would contain code something like this (way oversimplified) example:
my $error_msg;
try {
GateKeeper->validate($username, $password);
} catch LoginError with {
my $ex = shift;
$error_msg = GateKeeper->format_error($ex->{-text});
# Or better, SpecialFormatModule->format_error($ex->{-text});
}
(1)
Basically, you would trap exceptions raised by your validation code in GateKeeper, and then (taking seattlejohn's excellent advice), run that string through some formatting code so you don't have to worry about mixing HTML into your error messages.
Next you'd check if $error_msg was defined, and if so you'd know that something bad had happened. You wouldn't need to figure out what though, or do any more work, because you'd already have all the formatting done. Just print the page.
- For more robust exception handling you'd have the option of catching more specific subclasses rather than simply the lowest common denominator for any exception you expect to encounter here (e.g., LoginError). But for simplicity's sake...
| [reply]
[d/l]
[select] |
As Aristotle has pointed out, the HTML shouldn't be in your code. There are several ways to handle that. One is to not return anything that deals with formatting but instead have your program pass the results off to a template and the template decides the format. This has the advantage of keeping the formatting in the spot that it logically belongs (and if you use CSS, you can reap even greater benefits).
Another idea which I don't like as much, but which might be applicable for you, is to turn this class into a base class and have presentation classes that inherit from this and handle the formatting. I don't like that because it seems like bad design and isn't (in my mind) what inheritance is for. The benefit would be another method of controlling the presentation and it would allow you to override many of the constant values that you have hardcoded into the system. For example, if someone fails to login 3 times in a row, maybe you want the lockout time to be an hour. It all depends upon the business rules you're dealing with. Whenever you see hardcoded data like yours in a program, consider whether or not you'll ever need to override those values.
If you do wish to subclass this, you'll need to switch to the two-argument form of bless.
#constructor
sub new
{
my $class = shift;
my %self = map {$_ => undef} qw/user_id login_valid error_type err
+or_string/;
bless \%self, $class;
}
Cheers,
Ovid
New address of my CGI Course.
Silence is Evil | [reply]
[d/l] |
| [reply] |
Am I reading this right? If the user types in a bad username, you tell them. If the user types in a valid username but bad password, you tell them that as well. I've always been taught that doing so was a security risk. Try using a more generalized "Bad username and/or password".
| [reply] |