Hi Agnello,
a few more comments on your code:
On Monday 18 Apr 2011 18:31:30 Agnello George wrote:
> On Mon, Apr 18, 2011 at 6:17 PM, Shlomi Fish <shlo...@iglu.org.il> wrote:
> > Hi Agnello,
> >
> > some comments on your code.
> >
> > On Monday 18 Apr 2011 15:18:22 Agnello George wrote:
> > > Hi
> > >
> > > I am trying my hand in CGI , i have script that runs svn list
> > > on-submit which obviously takes 30 to 40 secs for the page to load , i
> > > plan to use
> >
> > a
> >
> > > ajax loader to be printed untill those 30 to 40 seconds. But even
> > > after the page has finished loading i can still see the image. I am
> > > not good in java script but can this be done using perl only .. here
> > > is a snip of my code ..
> >
> > Please include your entire code - not a snippet of it.
> >
> > > my @all_svn;
> > >
> > > unless (@all_svn) {
> >
> > This will always be executed since @all_svn is empty.
> >
> > > print '<p> <img alt="" src="
> >
> > http://192.168.1.25/template/ajax-loader.gif";
> >
> > > /></p>';
> > > }
> > >
> > > @all_svn = qx(svn list -R $virticals{$sitei}{svnurl}) ;
> >
> > 1. You've misspelled "verticals".
> >
> > 2. What is "sitei"?
> >
> > 3. Be careful from interpolating strings into qx/.../ :
> >
> > http://community.livejournal.com/shlomif_tech/35301.html
> >
> > > s/\s+$// for @all_svn;
> >
> > Subversion has Application Programmers Interfaces (APIs) for that, so you
> > don't need to parse its output non-reliably.
> >
> > > my %allsus = map {$_ => 1 } @filesi;
> >
> > What is "filesi"? What is "allsus"?
> >
> > > my @allgood = grep {defined $allsus{$_ } } @all_svn ;
> > > print "@allgood";
> >
> > Here you can have a cross site scripting attack:
> >
> > http://community.livejournal.com/shlomif_tech/35301.html
> >
> > Best Regards,
> >
> > Shlomi Fish
>
> I am sorry i had only posted a snip of my code , here is the whole code (
> which too is not complete ) .
>
> I am using template toolkit for output ( do you need me to paste my html
> code also )
>
> i have a simple UI , with a text area to input my files and a drop down
> menu to select which site i need to do deployment for .
>
> This code is not complete but among the many other issues I am facing ,
> the main issue is the ajax-loader.gif that i need to show on the UI whne
> the pages loads or on press of submit .
>
> #!/usr/bin/perl
>
> use strict ;
> use warnings ;
> use Template;
> use Data::Dumper;
> use CGI;
> my $query = new CGI;
>
That should be:
my $cgi = CGI->new;
1. Avoid indirect object notation:
http://perl-begin.org/tutorials/bad-elements/#indirect-object-notation
2. Call the CGI handle in something more descriptive than $query.
> print "Content-type: text/html\n\n";
>
After this, you will serve HTML regardless of what you wish to serve, so
serving an image won't be acceptable at all. CGI.pm has the $cgi->header()
method for that, BTW.
> my $tt = Template->new( INCLUDE_PATH => "/var/www/html/template" ) || die
> "template process failed:$!";
"||" here should be an "or".
>
> my %tag ;
> my %verticals = ( 'website1_v' => { 'tempdir' => '/temp/auto' , 'svnurl' =>
> 'http://svn.int.com/repos/branch/website1' } );
>
> my ($files, $site,$valu,$ma_status);
You've misspelt "value" as "valu" and you should have more spaces after the
comments.
> if ($ENV{'REQUEST_METHOD'} eq "POST") {
> $files = $query->param('element_1');
> $site = $query->param('mydropdown');
> $ma_status = gtfls_cfe("$files");
>
> }
>
This code seems sub-optimal. Perhaps you'd like a subroutine call or an
object. gtfls_cfe($files) (not
http://perl-begin.org/tutorials/bad-elements/#vars_in_quotes , mind) may give
you an XSS attack.
> sub gtfls_cfe {
> my (@files,$status);
> @files = split(/\s+/, $_[0]) ;
Don't predeclare variables:
http://perl-begin.org/tutorials/bad-elements/#declaring_all_vars_at_top
> $valu = check_if_exist_in_svn(\@files,$site);
>
Why isn't $valu (better written as "$value") declared here.
>
> if ($$valu{ireturn} == 1){
This is better written as $value->{ireturn}. Also in that case $value should
be an object with accessors:
http://perl-begin.org/tutorials/bad-elements/#accessing_object_slots_directly
> $status = " the following files not exist in svn <BR> ";
> $status .= "$_<BR>" foreach @{$$valu{notgood}};
"<BR>" should be "<br />" preferably (XHTML).
Furthermore, you have a potential XSS attack here.
> } elsif ($$valu{ireturn} == 0 ) {
> $status = " the following files are OK to deployed in production
> server <BR> ";
> # Do more code here
> #
> }
> return $status
Missing trailing semicolon. It is a good idea to have one everywhere (see Perl
Best Practices).
> }
>
> sub check_if_exist_in_svn {
> my (@filesi) = @{$_[0]};
> my ($sitei )= $_[1];
Don't use positional indexes into @_:
http://perl-begin.org/tutorials/bad-elements/#subroutine-arguments
> my @all_svn;
> #my $WW = '<p> <img alt="" src="
> http://192.168.1.25/template/ajax-loader.gif"; /></p>'; ### cant sem to
> make this work!!
What are you trying to do here.
> @all_svn = qx(svn list -R $verticals{$sitei}{svnurl}) ;
>
> s/\s+$// for @all_svn;
>
> my %allsus = map {$_ => 1 } @all_svn;
>
> my (@allgood,@notgood,%hash1 ) ;
>
> foreach (@filesi){
> if ( defined $allsus{$_} ){
> push (@allgood , $_);
> } elsif ( !defined $allsus{$_}) {
> push (@notgood, $_ ) ;
> }
>
>
> }
> if ( @notgood ) {
> %hash1 = ( 'ireturn' => 1 , notgood => \@notgood ) ;
> return (\%hash1) ;
> } elsif (@allgood){
> %hash1 = ( 'ireturn' => 0 , allgood => \@allgood ) ;
> return (\%hash1) ;
> }
>
Here you should use the Subversion API.
Regards,
Shlomi Fish
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
The Case for File Swapping - http://shlom.in/file-swap
Knuth is not God! God has already released TeX version 4.0.
Please reply to list if it's a mailing list post - http://shlom.in/reply .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
![http://192.168.1.25/template/ajax-loader.gif"](http://192.168.1.25/template/ajax-loader.gif"){kind=link}