Can this be rewritten better?

[Nikos]

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]

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

Please help with this. its a precide question

 

[Scott Bryce]

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.

 

[Nikos]

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.

 

[Henry Law]

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.

 

[Nikos]

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

 

[Henry Law]

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:aram 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]

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.

 

[Nikos]

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]

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




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]

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]

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.

 

[Henry Law]

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.

 

[Nikos]

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.

 

[Fabian Pilkowski]

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]

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

 

[Joe Smith]

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

 

[Nikos]

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')