Can this be rewritten better?

Discussion in 'Perl Misc' started by Nikos, Apr 29, 2005.

  1. Nikos

    Nikos Guest

    my $script = param('select') or "Ãñ÷éêÞ Óåëßäá!";

    $sth = $dbh->prepare( "SELECT host FROM guestlog" );
    $sth->execute();
    while( $row = $sth->fetchrow_hashref ) {
    if( $host eq $row->{host} ) {
    my $hostmatch = 1;
    }
    }

    if( param('select') and param('select') !~ /\.\./ )
    {
    open(FILE, "<../data/text/$script.txt") or die $!;
    my @data = <FILE>;
    close(FILE);

    my $data = join('', @data);

    $dbh->do( "UPDATE guestlog SET script='$script' WHERE host='$host'"
    ) or die $dbh->errstr;
    }
    elsif( $hostmatch == 1 )
    {
    $dbh->do( "UPDATE guestlog SET hostcount = hostcount + 1 WHERE
    host='$host'" ) or die $dbh->errstr;
    $sth = $dbh->prepare( "SELECT * FROM guestlog WHERE host='$host'" );
    $sth->execute();
    $row = $sth->fetchrow_hashref;

    $data = "Êáëþò Þëèåò " .$host. "! ×áßñïìáé ðïõ âñßóêåò ôçí óåëßäá
    åíäéáöÃñïõóá!\n" .
    "Ôåëåõôáßá öïñÜ Þñèåò åäþ ùò " .$row->{host}. " óôéò "
    ..$row->{date}. " !!\n" .
    "ÓýíïëéêÃò Þñèåò åäþ " .$row->{hostcount}. " öïñÃò!!!\n" .
    "Ôåëåõôáßá åßäåò ôï êåßìåíï { " .$row->{script}. " }\n" .
    "Ãïéü êåßìåíï èá ìåëåôÞóåòé áõôÞí ôçí öïñÜ !?";
    }
    elsif( $hostmatch != 1 )
    {
    if ( $host ne "Ãßêïò" )
    {
    $data = "ÃåéÜ óïõ " .$host. "!\n" .
    "¸ñ÷åóáé ãéá 1ç öïñÜ åäþ !!\n" .
    "Åëðßæù íá âñåßò ôá êåßìåíá åíäéáöÃñïíôá :)";

    $dbh->do( "INSERT INTO guestlog VALUES (null, '$host', '$date',
    '$script', 1, 1)" ) or die $dbh->errstr;
    }
    else
    {
    $data = "ÃåéÜ óïõ Ãéêüëá, ôé ÷áìðÜñéá?! ¼ëá äåîéÜ íá óïõ ðÜíå
    ðÜíôá! ;-)";
    }
    }

    ok with the above script iam trying to see if the current hostname of a
    visitor exists in guestlog mysql database.

    If it does then i just update the existing record,
    if it does not i just welcome the user.


    Please you asked me to ask specific questions and here i ask one:

    Is the above logic correct?
    Is there any betetr that i can write it?

    ps. Sorry about the Greek folks but although i use UTF-8 it still being
    displayed like shit. I hate seeing it myself too but i donw know how can
    they be displayed correctly.
    Nikos, Apr 29, 2005
    #1
    1. Advertising

  2. Nikos

    Nikos Guest

    Nikos wrote:
    > my $script = param('select') or "Ãñ÷éêÞ Óåëßäá!";


    Please help with this. its a precide question
    Nikos, Apr 29, 2005
    #2
    1. Advertising

  3. Nikos

    Scott Bryce Guest

    Nikos wrote:

    > Nikos wrote:
    >
    >> my $script = param('select') or "Ãñ÷éêÞ Óåëßäá!";

    >
    >
    > Please help with this. its a precide question


    What do you expect this line to do? What is it doing that is different
    than you expect? Please write it into a short, but complete program that
    demonstrates the problem.

    And read the posting guidelines for this group. It has got to be less
    reading that reading all of the posts that have tried to explain them to
    you.
    Scott Bryce, Apr 29, 2005
    #3
  4. Nikos

    Nikos Guest

    Scott Bryce wrote:
    > Nikos wrote:
    >
    >> Nikos wrote:
    >>
    >>> my $script = param('select') or "Ãñ÷éêÞ Óåëßäá!";


    I meant [snip] the rest!

    here is the code so far that i want to be rewritten in a better shorter way!

    my $script = param('select') or "Ãñ÷éêÞ Óåëßäá!";

    $sth = $dbh->prepare( "SELECT host FROM guestlog" );
    $sth->execute();

    my $hostmatch;
    while( $row = $sth->fetchrow_hashref ) {
    if( $host eq $row->{host} ) {
    $hostmatch = 1;
    }
    }

    my ($data, @data);
    if( param('select') and param('select') !~ /\.\./ )
    {
    open(FILE, "<../data/text/$script.txt") or die $!;
    @data = <FILE>;
    close(FILE);

    $data = join('', @data);

    $dbh->do( "UPDATE guestlog SET script='$script' WHERE host='$host'"
    ) or die $dbh->errstr;
    }
    elsif( $hostmatch == 1 )
    {
    $dbh->do( "UPDATE guestlog SET hostcount = hostcount + 1 WHERE
    host='$host'" ) or die $dbh->errstr;
    $sth = $dbh->prepare( "SELECT * FROM guestlog WHERE host='$host'" );
    $sth->execute();
    $row = $sth->fetchrow_hashref;

    $data = "Êáëþò Þëèåò " .$host. "! ×áßñïìáé ðïõ âñßóêåò ôçí óåëßäá
    åíäéáöÃñïõóá!\n" .
    "Ôåëåõôáßá öïñÜ Þñèåò åäþ ùò " .$row->{host}. " óôéò "
    ..$row->{date}. " !!\n" .
    "ÓýíïëéêÃò Þñèåò åäþ " .$row->{hostcount}. " öïñÃò!!!\n" .
    "Ôåëåõôáßá åßäåò ôï êåßìåíï { " .$row->{script}. " }\n" .
    "Ãïéü êåßìåíï èá ìåëåôÞóåòé áõôÞí ôçí öïñÜ !?";
    }
    elsif( $hostmatch != 1 )
    {
    if ( $host ne "Ãßêïò" )
    {
    $data = "ÃåéÜ óïõ " .$host. "!\n" .
    "¸ñ÷åóáé ãéá 1ç öïñÜ åäþ !!\n" .
    "Åëðßæù íá âñåßò ôá êåßìåíá åíäéáöÃñïíôá :)";

    $dbh->do( "INSERT INTO guestlog VALUES (null, '$host', '$date',
    '$script', 1, 1)" ) or die $dbh->errstr;
    }
    else
    {
    $data = "ÃåéÜ óïõ Ãéêüëá, ôé ÷áìðÜñéá?! ¼ëá äåîéÜ íá óïõ ðÜíå
    ðÜíôá! ;-)";
    }
    }

    Please help me.
    This is the best way i can write it.
    Nikos, Apr 29, 2005
    #4
  5. Nikos

    Henry Law Guest

    On Fri, 29 Apr 2005 17:25:38 +0300, Nikos <> wrote:

    >my $script = param('select') or "Áñ÷éêÞ Óåëßäá!";


    What happens if param fails for some reason?

    >
    >$sth = $dbh->prepare( "SELECT host FROM guestlog" );
    >$sth->execute();
    >while( $row = $sth->fetchrow_hashref ) {
    > if( $host eq $row->{host} ) {
    > my $hostmatch = 1;


    The use of "my" here will localise $hostmatch to the braces of the
    "if" statement. It will be undef when you need it later.

    > }
    >}
    >
    >if( param('select') and param('select') !~ /\.\./ )


    I don't know what this is doing but it looks odd to me. I can't
    imagine what the two calls to "param" are returning which when anded
    together could ever look like ".."

    Oh, and code your left brace at the end of the line, as you've done
    before

    >{
    > open(FILE, "<../data/text/$script.txt") or die $!;
    > my @data = <FILE>;
    > close(FILE);


    Can you be sure that $script.txt isn't too big to be loaded into
    memory all at once?

    >
    > my $data = join('', @data);
    >
    > $dbh->do( "UPDATE guestlog SET script='$script' WHERE host='$host'"
    >) or die $dbh->errstr;
    >}
    >elsif( $hostmatch == 1 )


    See comment on $hostmatch earlier

    >{
    > $dbh->do( "UPDATE guestlog SET hostcount = hostcount + 1 WHERE
    >host='$host'" ) or die $dbh->errstr;
    > $sth = $dbh->prepare( "SELECT * FROM guestlog WHERE host='$host'" );
    > $sth->execute();
    > $row = $sth->fetchrow_hashref;
    >
    > $data = "Êáëþò Þëèåò " .$host. "! ×áßñïìáé ðïõ âñßóêåò ôçí óåëßäá
    >åíäéáöÝñïõóá!\n" .
    > "Ôåëåõôáßá öïñÜ Þñèåò åäþ ùò " .$row->{host}. " óôéò "
    >.$row->{date}. " !!\n" .
    > "ÓýíïëéêÝò Þñèåò åäþ " .$row->{hostcount}. " öïñÝò!!!\n" .
    > "Ôåëåõôáßá åßäåò ôï êåßìåíï { " .$row->{script}. " }\n" .
    > "Ðïéü êåßìåíï èá ìåëåôÞóåòé áõôÞí ôçí öïñÜ !?";
    >}
    >elsif( $hostmatch != 1 )


    This is just a simple "else" construct; you've checked to see whether
    $hostmatch is 1, so now it's not.

    >{
    > if ( $host ne "Íßêïò" )
    > {
    > $data = "ÃåéÜ óïõ " .$host. "!\n" .
    > "¸ñ÷åóáé ãéá 1ç öïñÜ åäþ !!\n" .
    > "Åëðßæù íá âñåßò ôá êåßìåíá åíäéáöÝñïíôá :)";
    >
    > $dbh->do( "INSERT INTO guestlog VALUES (null, '$host', '$date',
    >'$script', 1, 1)" ) or die $dbh->errstr;
    > }
    > else
    > {
    > $data = "ÃåéÜ óïõ Íéêüëá, ôé ÷áìðÜñéá?! ¼ëá äåîéÜ íá óïõ ðÜíå
    >ðÜíôá! ;-)";
    > }
    >}
    >
    >ok with the above script iam trying to see if the current hostname of a
    >visitor exists in guestlog mysql database.
    >
    >If it does then i just update the existing record,
    >if it does not i just welcome the user.


    I have no idea about the logic. Tell you what: you run it and see if
    it does what you expect. If it doesn't, try to figure out why.

    >Please you asked me to ask specific questions and here i ask one:
    >
    >Is the above logic correct?


    No idea. That's not a Perl question. It's a program design
    question, which is better answered somewhere else (if you can find
    somewhere).

    >Is there any betetr that i can write it?


    I've marked a few things; I advise you just to make it work and don't
    worry too much about the programming style as long as it is decently
    laid out.

    >ps. Sorry about the Greek folks but although i use UTF-8 it still being
    >displayed like shit. I hate seeing it myself too but i donw know how can
    >they be displayed correctly.


    I'm not expert in that, but I don't think that changing your code page
    will help; it's not going to map "beta" into b, or "rho" into r is it?

    But if you'd make a SMALL, SELF-CONTAINED program - as it asks you to
    in the posting guidelines - then you could use only a-z just for that
    and the problem wouldn't arise.
    Henry Law, Apr 29, 2005
    #5
  6. Nikos

    Nikos Guest

    Henry Law wrote:

    The script is working ok now that i removes my as you correclty said but
    i think maybe it can be even better written
    Nikos, Apr 29, 2005
    #6
  7. Nikos

    Henry Law Guest

    On Fri, 29 Apr 2005 19:59:35 +0300, Nikos <> wrote:

    >Please help me.
    >This is the best way i can write it.


    Are you asking us to comment on your coding style or the logic of your
    program? I don't think anyone is going to debug your program for you.
    Even if we were (and I did think of trying, just to be helpful) we
    can't run it. Look:

    F:\WIP>perl nikos.pl
    Undefined subroutine &main::param called at nikos.pl line 1.

    F:\WIP>

    It compiles clean, of course, because it has no strict and warnings.
    I added them and this happened, as expected:

    F:\WIP>perl nikos.pl
    Global symbol "$sth" requires explicit package name at nikos.pl line
    6.
    Global symbol "$dbh" requires explicit package name at nikos.pl line
    6.
    Global symbol "$sth" requires explicit package name at nikos.pl line
    7.

    .... and so on and on. That's because I haven't got the requisite DBI
    module (I even coded "use DBI;" for you just in case.

    Nikos, let me say this once more: we WILL NOT DEBUG YOUR PROGRAM LOGIC
    FOR YOU. That's not what this group is for.

    If your logic goes wrong because you've not understood how Perl works,
    that we can help with.
    Henry Law, Apr 29, 2005
    #7
  8. Nikos

    Henry Law Guest

    On Fri, 29 Apr 2005 20:18:35 +0300, Nikos <> wrote:

    >Henry Law wrote:


    No, actually, I didn't. You did.

    >The script is working ok now that i removes my as you correclty said but
    >i think maybe it can be even better written


    My advice to you is not to bother about coding style; you have more
    serious problems.
    Henry Law, Apr 29, 2005
    #8
  9. Nikos

    Nikos Guest

    Jim Gibson wrote:

    By following almost all your sugegstions i convert it to:


    ===========================================================
    my $script = param('select') or "Welcome Page!";

    $sth = $dbh->prepare( "SELECT host FROM guestlog WHERE host='$host'" );
    $sth->execute();

    my ($data, @data);
    if( param('select') and param('select') ne '..' )
    {
    open(FILE, "<../data/text/$script.txt") or die $!;
    @data = <FILE>;
    close(FILE);

    $data = join('', @data);

    $dbh->do( "UPDATE guestlog SET script='$script' WHERE host='$host'"
    ) or die $dbh->errstr;
    }
    elsif( $sth->rows )
    {
    $dbh->do( "UPDATE guestlog SET hostcount = hostcount + 1 WHERE
    host='$host'" ) or die $dbh->errstr;
    $sth = $dbh->prepare( "SELECT * FROM guestlog WHERE host='$host'" );
    $sth->execute();
    $row = $sth->fetchrow_hashref;

    $data = "Never mind about that";
    }
    elsif( not $sth->rows )
    {
    if ( $host ne "Íßêïò" ) {
    $data = "Never mind about that";

    $dbh->do( "INSERT INTO guestlog VALUES (null, '$host', '$date',
    '$script', 1, 1)" ) or die $dbh->errstr;
    }
    else {
    $data = "Never mind about that its greek";
    }
    }
    ===========================================================


    Dont worry about param('select') it gets its value.

    I decided i dont really need hostmatch.

    Can we make it even shorter and better written?
    Nikos, Apr 29, 2005
    #9
  10. Nikos

    Nikos Guest

    Henry Law wrote:
    > On Fri, 29 Apr 2005 20:18:35 +0300, Nikos <> wrote:
    >
    >
    >>Henry Law wrote:

    >
    >
    > No, actually, I didn't. You did.
    >
    >
    >>The script is working ok now that i removes my as you correclty said but
    >>i think maybe it can be even better written

    >
    >
    > My advice to you is not to bother about coding style; you have more
    > serious problems.


    The script is working but i want to write it as betetr as possible.
    ima learnign this way.
    Nikos, Apr 29, 2005
    #10
  11. Nikos

    Nikos Guest

    Tim Hammerquist wrote:
    > Nikos <> wrote:
    >
    >> my $script = param('select') or "Áñ÷éêÞ Óåëßäá!";

    >
    >
    > If you want to set $script to "Áñ÷éêÞ Óåëßäá!" if param('select') is
    > empty, you'll probably want to use the || operator instead of "or". As
    > it is, $script will either be set to param('select') or undef.
    >
    > HTH,
    > Tim Hammerquist


    Thank you Tim, but why is this happening?

    || and or are functioning the same way as far as i know.
    Nikos, Apr 29, 2005
    #11
  12. Nikos

    Nikos Guest

    Tim Hammerquist wrote:
    > Nikos <> wrote:
    >
    >>Can we make it even shorter and better written?

    >
    >
    > You sure can. You might want to read up on Perl some more. The best
    > way to learn to program fast, efficient Perl code is study, practice,
    > and lurking in these here newsgroups.
    >
    > Posting code and asking if there's a better way is not a good approach.
    >
    > Yes, there's probably a better way to write it. And for a standard fee,
    > many people here would be happy to show you.


    Help a newbie man, i cant afford paying....iam trying
    and laerning by asking and finding better ways of writing the code.
    please.
    Nikos, Apr 29, 2005
    #12
  13. Nikos

    Henry Law Guest

    On Fri, 29 Apr 2005 21:16:24 +0300, Nikos <> wrote:

    >Tim Hammerquist wrote:
    >> Posting code and asking if there's a better way is not a good approach.
    >>


    >Help a newbie man, i cant afford paying....iam trying
    >and laerning by asking and finding better ways of writing the code.


    The code works (you said); it's not terrible; don't change anything
    more - move on to something else.
    Henry Law, Apr 29, 2005
    #13
  14. Nikos

    Nikos Guest

    Henry Law wrote:

    > The code works (you said); it's not terrible; don't change anything
    > more - move on to something else.


    Iam under the idea of making it as shorter, as clearer and as
    strightforward as it can gets.
    O cant relax until this is done.
    Nikos, Apr 29, 2005
    #14
  15. * Nikos schrieb:
    > Tim Hammerquist wrote:
    >>
    >>> my $script = param('select') or "Áñ÷éêÞ Óåëßäá!";

    >>
    >> If you want to set $script to "Áñ÷éêÞ Óåëßäá!" if param('select') is
    >> empty, you'll probably want to use the || operator instead of "or". As
    >> it is, $script will either be set to param('select') or undef.

    >
    > Thank you Tim, but why is this happening?
    > || and or are functioning the same way as far as i know.


    Well, as far as *you* know. Tim has told you that there's a difference
    between »||« and »or«. Why do you ask here? Please have a look into the
    docs, it is mentioned in `perldoc perlop` since both are operators.

    You have said you will learn by asking here. But -- and please think
    about that -- what should we do to help you? Should I quote the docs for
    you now? I don't think that's really necessary.

    regards,
    fabian
    Fabian Pilkowski, Apr 29, 2005
    #15
  16. * Nikos schrieb:
    >
    > The script is working but i want to write it as betetr as possible.
    > ima learnign this way.


    But, and I hope you will realize this soon, here is no one for teaching
    you this way.

    First you've posted your complete scripts here, large and unclear. Then
    you've heard that you should ask *specific questions* if you want some
    help. And what are you doing? You're just posting small unchanged chunks
    of your script to get some better versions. That's not correct.

    Please read the posting guidelines for this group again! Btw, have you
    ever read them really?

    Last, it is a bad idea to get *nicer* code from some people here with
    your skill level. Your code is good enough if *you* understand it.

    regards,
    fabian
    Fabian Pilkowski, Apr 29, 2005
    #16
  17. Nikos

    Joe Smith Guest

    Henry Law wrote:

    >>if( param('select') and param('select') !~ /\.\./ )

    >
    > I don't know what this is doing but it looks odd to me. I can't
    > imagine what the two calls to "param" are returning which when anded
    > together could ever look like ".."


    They're not anded together. To perform an AND operation on two
    integers require using & not && or 'and'. The quoted line translates
    to "if param('select') is true (not undef, not '', not 0, not '0')
    and the value does not contain two consecutive periods ...".

    It probably makes more sense to write it as
    if (defined param('select') and param('select') ~= /\.\./) { ... }

    -Joe
    Joe Smith, Apr 30, 2005
    #17
  18. Nikos

    Nikos Guest

    Joe Smith wrote:
    > Henry Law wrote:
    >
    >>> if( param('select') and param('select') !~ /\.\./ )

    >>
    >>
    >> I don't know what this is doing but it looks odd to me. I can't
    >> imagine what the two calls to "param" are returning which when anded
    >> together could ever look like ".."

    >
    >
    > They're not anded together. To perform an AND operation on two
    > integers require using & not && or 'and'. The quoted line translates
    > to "if param('select') is true (not undef, not '', not 0, not '0')
    > and the value does not contain two consecutive periods ...".
    >
    > It probably makes more sense to write it as
    > if (defined param('select') and param('select') ~= /\.\./) { ... }
    >
    > -Joe


    I wrote it like this to avoid getting hackers pass cgi parameters to
    open files from the adress bar like:

    http://www.nikolas.tk/cgi-bin/index.pl?select='../../somepath/somefile') :)

    --
    I knew UNIX before it was spelled L I N U X
    and
    I knew INTERNET before it was spelled W W W
    Nikos, Apr 30, 2005
    #18
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Alan Silver

    How to allow postback with a rewritten URL

    Alan Silver, Dec 1, 2005, in forum: ASP .Net
    Replies:
    0
    Views:
    449
    Alan Silver
    Dec 1, 2005
  2. Berthold Höllmann

    Can (should) this be rewritten without exec?

    Berthold Höllmann, Jun 9, 2004, in forum: Python
    Replies:
    1
    Views:
    231
    Terry Reedy
    Jun 9, 2004
  3. Harald Armin  Massa

    reddit.com rewritten in Python

    Harald Armin Massa, Dec 5, 2005, in forum: Python
    Replies:
    2
    Views:
    290
    Jay Parlar
    Dec 5, 2005
  4. Peter Hansen

    Re: reddit.com rewritten in Python

    Peter Hansen, Dec 5, 2005, in forum: Python
    Replies:
    1
    Views:
    305
    BartlebyScrivener
    Dec 11, 2005
  5. garey
    Replies:
    5
    Views:
    131
    Thomas 'PointedEars' Lahn
    Nov 19, 2009
Loading...

Share This Page