'lo

barry bazza at bazza.com
Mon Aug 27 08:31:00 CDT 2001


On Mon, Aug 27, 2001 at 02:12:38PM +0100, Andrew Wilson mumbled:
> Hi bazza,
> 
> Well, since you've asked ... I've had a look a photos.html

<snipped http://bazza.com/code/photos.html>

> Some of these are minor niggles, this one isn't you havent used
> strict. that's almost always a mistake.

that's generally a major failing of mine, it's not something I've ever managed
to get into the habit of doing

> You don't name the variable that you use to control the loop but then
> you dont take proper advantage of $_ either.
> either do 
> foreach (<*>) {
>   next if /~/;
>   next if m#.cgi$#;
>   if (m#.jpg#) {
>     ...

this is the way I'd like to do things, but for some odd reason I seem to do
somthing wrong and it never works if I don't directly use $_


> I don't like your regular expressions they match too much.  For
> instance /cgi/ matches map_of_dirs_containg_cgi_files.jpg and lots of
> other things that you don't really want.
> /\.cgi$/ is a much tighter and hence much less likely to false match
> regexp.
> same with /jpg/,  should be /\.jpg$/  or if you don't like /\ use
> m instead m#.cgi$# and m#.jpg$#

again, sort of similar to above, it's sloppiness, never getting into habits of
things, and not doing enough work with this stuff

> This is a style thing and you can feel free to ignore it.  I don't
> like long lines like the ones you have in your HTML.  You can break
> the HTML on whitespace and I think that makes it easier to read.
> you aren't taking proper advantage of perls quoting operators either.
> You have a habit of doing "I want quotes in here so I'll do this \""
> when what you should be doing is 
> qq{I want quotes in here so I'll do this "}

I didn't even know about that

> There is no real need for the subroutines unless you put them in some
> other file and import them.  Much better to use a templating system
> have a look at Template toolkit, it's excellent.
> This is definitely a problem, you have identical code in both halves
> of the if/else statement.  You should extract it.  Also you weren't
> closing the <tr> tag if there was no comment.

Yeah, templating is something I need to poke around at a lot, even for my main
page it'd come in a lot more useful than that stuff I'm doing now, another big
part of the problem is that once I write stuff, I /very/ rarely go back to it
to clean it up, or implement things I've learnt since that have improved
coding, the webcam cgi is probably my latest of the things on there, which
doesn't use subroutines, and does put different things in depending on how
it's called or what it's meant to be doing, shall save this and have a pour
over it .. thanks

-- 
-Barry Hughes
An unbreakable toy is useful for breaking other toys
                             http://bazza.com/



More information about the Belfast-pm mailing list