in reply to How to write good code?
Please don't insert line numbers when posting code here. If people want to see line numbers, they can turn them on in their Display Settings.
Sub declarations like sub constructList($release, $daily_test) don't work, at least not in Perl 5 (but maybe you were just showing that as pseudocode).
'\n' does not result in a string containing a newline character, as \n is not special in single-quoted strings. But remember that the first argument of split is a regex, so it is probably clearer to write split /\n/ anyway.
== is not for comparing strings, only numbers (as a previous respondent said). Use eq for comparing strings.
You use [a] in a regex. That is a character class which matches the single letter a. I suspect that you meant the character class [a-z].
You assign $1 to $extracted, and then you use $1. You should use $extracted.
Use modules where possible. Don't reinvent the wheel, let alone the Space Shuttle.
I might rewrite your code as:
my %add = ( daily => \&addDaily, test => \&addTest, ); $add{$daily_or_test} or return; # should die instead? my @List; for ( split /\n/, $res->as_string ) { if ( my( $extracted ) = /href="([a-z][^\/]*)/i ) { $extracted =~ /win/i and $add{$daily_or_test}->( $extracted ) == 1 and push @List, $extracted; } }
But since it looks like you're dealing with HTML, I'd use an HTML parser module, such as HTML::TreeBuilder.
Also, instead of passing in a string which symbolically indicates whether we're doing a daily or a test, you could pass in the function (by reference) directly:
ref($daily_or_test) eq 'CODE' or die; . . . $daily_or_test->( $extracted ) . . .
|
|---|