Re: [code-review] WebService::Validator::HTML::W3C

On Fri, Nov 14, 2003 at 05:01:20PM +0000, Struan Donald wrote:
> Any thoughts/improvements/whatever on the attached?

    if ( $v->validate("http://www.example.com/") ) {
        if ( $v->valid ) {
            printf ("%s is a valid website\n", $v->uri);
            } else {
            printf ("%s is not a valid website\n", $v->uri);
            foreach $error ( $v->errors ) {
                printf("%s at line %n\n", $error->{description},
                                          $error->{line_no});
            }
        }
    } 

As well as the formatting of this being a little wonky, I would also
find this misleading as a first time user. Your example output of "a
valid website" implies that it's going to crawl and validate the
entire site, when it appears to only do a single URI.

I'd change the example prints to "validates" and "does not validate"
(for bonus marks I'd say what it validates or doesn't validate _as_:

  %s does not validate as XHTML 1.0/Strict

--------

The validate() method is a little big for my liking. If I wanted to
subclass this to change some behaviour, I'd have to cut'n'paste this
entire method. So, if for example, I wanted to use a different validator
service, which needed a slightly differently formatted URL, or returned
slightly different headers, I couldn't just override the small portion of
the code that dealt with that. Each discreet 'concept' here should
really be a distinct method so that people can supply their own version
of that concept.

--------

You have a few chunks of code of the form:

    unless ( $uri ) {
        $self->validator_error("You need to supply a URI to validate");
        return 0;
    }

As far as I can see, every time you set the validator error you
immediately return zero.

I'd probably change validator_error() to return zero, and replace all
these with code like:

	return $self->validator_error("...") unless $uri;

--------

Personally I also dislike needless nesting. So, for example, in errors()
you have:

  if ($@) {
    warn & return
  } else {
    main body of code.
  }

I find this easier to follow as:

	if ($@) {
    warn & return
  }
  main body of code.

But that's hardly a major point! My main dislike with this method, from
the outside, rather than the inside, is that it returns a listref of
hashrefs. The rest of the module takes an OO approach, and then this
drops back to just returning a complex data structure. I'd probably make
this return a list of Validator::Error objects that can have methods for
the things you want. For bonus marks I'd also have the object be able to
show you the line in question rather than just tell you the line number.

-------


Tony