carlriz has asked for the wisdom of the Perl Monks concerning the following question:

Hey guys! I am completely new to this site, and I started learning perl a couple of weeks ago. I find it fascinating! So, the reason why I am here is to get some input on the script I wrote... Criticize and critique...How can I make this script better?



#!/usr/bin/perl                                                       
+                                                                     
+                                          

use 5.012;
use warnings;
use threads;
use threads::shared;
use Thread::Queue;
use LWP::UserAgent;
use Time::HiRes qw( time );
use constant THREADS => 50;
use Data::Dumper;

$| = 1;

#########                                                             
+                                                                     
+                                          
# Main  #                                                             
+                                                                     
+                                          
#########                                                             
+                                                                     
+                                          

my $queue = Thread::Queue->new();
my @URLs =  qw( http://www.google.com/)x50;
my @threads;
my $time = time;
my $json = '{"username": "anonymous", "password":"anonymous"}';
my %query_hash = (action => 'submit or status', name => 'chris',  outn
+ame => 'chris', 'content-type' => 'application/json');

print"\n\ Test...\n\n";


my $counter_thread : shared;

for (1..THREADS) {
    push @threads, threads->create(sub {
        my $ua = LWP::UserAgent->new;
        $ua->timeout(5); # short timeout for easy testing.            
+                                                                     
+                                          
        while(my $task = $queue->dequeue) {

            my $request = HTTP::Request->new(POST=>$task);
            $request->header(%query_hash);
            $request->content($json);

            my $response = $ua->request($request);;

            print"\n\n", $counter_thread++,"th :", "Dumped Data Return
+ed:\n\n ", Dumper($response->decoded_content), "\n" ;

            print "Response code:  ", $response->code, "\tHTTP Respons
+e Message: ",
            $response->message, " ", "\n";
        }
       });
}

$queue->enqueue(@URLs);
$queue->enqueue(undef) for 1..THREADS;



Pending_HTTP();
Print_File();


{$_-> join foreach @threads};

##############################################                        
+                                                                     
+                                          
# Print pending HTTP 'POST' requests to file #                        
+                                                                     
+                                          
##############################################                        
+                                                                     
+                                          
sub Pending_HTTP {
    my $file_write = 'Out_Stats.txt';
    open(FILE, '>', $file_write) or die $!;

    while (!($queue->pending() == 0)) {

        sleep(1);
        my $time_now = time;
        print FILE "Number of pending HTTP 'POST' Requests: ", $queue-
+>pending(),
        "\tTime Elapsed:  ", ($time_now - $time), "\n";
    }

    close(FILE);
}

#################################                                     
+                                                                     
+                                          
# Print file to standard output #                                     
+                                                                     
+                                          
################################# 


sub Print_File {
    print "\n\nListing Pending HTTP Requests and Time elapsed...\n\n";
    open(FILE, '<', 'Out_Stats.txt') or die $!;

    print "**********************************************\n\n";

    my $i = 1;
    while(defined($_ = <FILE>)) {

        print "$i: ", $_;
        $i++;
    }

    close(FILE);
}

[download]

Replies are listed 'Best First'.
Re: HTTP Requests and Threading
by kcott (Archbishop) on Mar 06, 2014 at 19:25 UTC

    G'day carlriz,

    Welcome to the monastery.

    In the main, this looks like a well-written piece of code. Were you just looking for comments on the code or are you experiencing some difficulties (e.g. something doesn't work as expected, excessive run time, chews up lots of memory, etc.)?

    If the 'x50' (in my @URLs = ...) is related to the THREADS constant, then 'x THREADS' would be a better choice.

    I'd probably set the timeout value as a constant (near the top of the script) and keep comments relating to that value there. The current comment refers to testing so presumably that's intended to change during the development cycle. Consider the chances of forgetting to search through the script for $ua->timeout(5) and changing it, against the chances of not spotting TIMEOUT => 5, # for testing near the top; you also won't have to search for $ua->timeout(TIMEOUT) (however many times it occurs) as it won't need modification.

    Overall, the layout looks very good and is easy to read. I'd aim to keep lines <= 80 characters (others have different preferences: 72 and 78 characters are common). When lines start to get much longer than this, readability suffers. Consider the readability of one of your lines and the suggested alternative following it: 

    my %query_hash = (action => 'submit or status', name => 'chris',  outn
+ame => 'chris', 'content-type' => 'application/json');

    [download]
    my %query_hash = (
    action         => 'submit or status',
    name           => 'chris',
    outname        => 'chris',
    'content-type' => 'application/json'
);

    [download]

    Finally, of minor importance and I suspect this may be an artefact of your editor, you have a large amount of extraneous whitespace after many of you lines. Because this is wrapping in my browser (and I suspect this will be true for many others), I actually see your intended 

    #########
# Main  #
#########

    [download]

    as

    #########
+
# Main  #
+
#########
+

    [download]

    There's many examples of that wrapping issue throughout the code you posted.

    -- Ken

[reply]
[d/l]
[select]

      Hi ken, Thanks for the thorough message. I am having difficulties with outputting my data to stdout, in the format that I wanted. The way I wrote it is when the individual threads complete, they execute this code: 

      
 if( $response->is_success ) {

            print"\n\n", $counter_thread++,"th :", "Dumped Data Return
+ed:\n\n ", Dumper($response->decoded_content), "\n" ;

            print "Response code:  ", $response->code, "\n",
            "HTTP Response Message: ",
            $response->message, " ", "\n";

           } else {
             print STDERR $response->status_line, "\n";
          }

      [download]

      However, they come out in unsorted order. I am trying to print each thread in order with time elapsed for each thread, decoded_content in a neat way. As an example, the variable '$counter_thread' would print like 2,5,8, 10,7, etc. to stdout. Is this a threading issue? What is the best way to go about this? Chris

[reply]
[d/l]

        Your threads run concurrently and will take different times to complete depending upon a bunch of factors -- the remote server's workload; distance and latency of the link between your machine and those servers; the size of the page returned; etc. -- so the order in which they finish will be unrelated to the order that you start them. That is normal.

        If you want to control the order of the output, you will need to accumulate the results and arrange to output them in your desired order.

        With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.
[reply]
[reply]
Re: HTTP Requests and Threading
by kcott (Archbishop) on Mar 06, 2014 at 19:53 UTC

    A further point has just occurred to me.

    Don't hard-code values in your subroutines, pass those values when calling. The reasons are much the same as I gave for TIMEOUT in my earlier comments.

    Change

    sub Pending_HTTP {
    my $file_write = 'Out_Stats.txt';
    ...
}

    [download]

    to

    sub Pending_HTTP {
    my ($file_write) = @_;
    ...
}

    [download]

    (and similarly for sub Print_File {...})

    The calling code would then become something like:

    my $outfile = 'Out_Stats.txt';
...
Pending_HTTP($outfile);
Print_File($outfile);

    [download]

    If the filename changes, you'll only need to change the $outfile assignment; currently, you'd need to search and replace every occurrence of 'Out_Stats.txt'.

    -- Ken

[reply]
[d/l]
[select]

      That's a good tip. Thanks

[reply]
Re: HTTP Requests and Threading
by zentara (Cardinal) on Mar 07, 2014 at 11:34 UTC
[reply]
Re: HTTP Requests and Threading
by crusty_collins (Friar) on Mar 06, 2014 at 19:04 UTC
    use strict;
[reply]
      "use strict;"

      use 5.012; (at the top of the script) already does this.

      From use:

      "... if the specified Perl version is greater than or equal to 5.12.0, strictures are enabled lexically as with use strict."

      -- Ken

[reply]
[d/l]
[select]

        Well, that's obvious.

        use joke 14; - tye        

[reply]
Re: HTTP Requests and Threading
by mje (Curate) on Mar 07, 2014 at 09:08 UTC
[reply]

Back to Seekers of Perl Wisdom