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 <shlomif [at] 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 [at] all_svn;
> > >
> > > unless ( [at] all_svn) {
> >
> > This will always be executed since [at] all_svn is empty.
> >
> > > print '<p> <img alt="" src="
> >
> > http://192.168.1.25/template/ajax-loader.gif"
> >
> > > /></p>';
> > > }
> > >
> > > [at] 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 [at] 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 } [at] filesi;
> >
> > What is "filesi"? What is "allsus"?
> >
> > > my [at] allgood = grep {defined $allsus{$_ } } [at] all_svn ;
> > > print " [at] 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-objec t-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 ( [at] files,$status);
> [at] 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(\ [at] 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_obje ct_slots_directly
> $status = " the following files not exist in svn <BR> ";
> $status .= "$_<BR>" foreach [at] {$$valu{notgood}};
"<BR>" should be "
" 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 ( [at] filesi) = [at] {$_[0]};
> my ($sitei )= $_[1];
Don't use positional indexes into [at] _:
http://perl-begin.org/tutorials/bad-elements/#subroutine-arg uments
> my [at] 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.
> [at] all_svn = qx(svn list -R $verticals{$sitei}{svnurl}) ;
>
> s/\s+$// for [at] all_svn;
>
> my %allsus = map {$_ => 1 } [at] all_svn;
>
> my ( [at] allgood, [at] notgood,%hash1 ) ;
>
> foreach ( [at] filesi){
> if ( defined $allsus{$_} ){
> push ( [at] allgood , $_);
> } elsif ( !defined $allsus{$_}) {
> push ( [at] notgood, $_ ) ;
> }
>
>
> }
> if ( [at] notgood ) {
> %hash1 = ( 'ireturn' => 1 , notgood => \ [at] notgood ) ;
> return (\%hash1) ;
> } elsif ( [at] allgood){
> %hash1 = ( 'ireturn' => 0 , allgood => \ [at] 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-unsubscribe [at] perl.org
For additional commands, e-mail: beginners-help [at] perl.org
http://learn.perl.org/
