How to make it shorter and better?

Hello,,,

I don't know much about PHP+SQL but I have to find the better way to make my
query work and to be shorter and better.
All help will be appreciated.

//Check if IP exists in banIP
$res = mysql_query("SELECT ip FROM banIP WHERE ip =
'".$_SERVER['REMOTE_ADDR']."'");
$res = mysql_num_rows($res);
if($res != 0)
die;
mysql_free_result($res);

//Fetch Time
$timestamp = time();
$timeout = $timestamp - 900;
//Delete Users
$del = mysql_query("DELETE FROM caspionet WHERE timestamp<$timeout");

//Check if IP exists in caspionet
$res = mysql_query("SELECT ip FROM caspionet WHERE ip =
'".$_SERVER['REMOTE_ADDR']."'");
$res = mysql_num_rows($res);
if($res == 0){
//Insert User
$ins = mysql_query("INSERT INTO caspionet (timestamp, ip)
VALUES('$timestamp','".$_SERVER['REMOTE_ADDR']."')");
}
mysql_free_result($res);

//Fetch Users Online
$res = mysql_query("SELECT DISTINCT ip FROM caspionet");
while ($row = mysql_fetch_array($res, MYSQL_BOTH)){
print("IP:$row[0]");
}
mysql_free_result($res);

Thank you
A.K.

I won't make it shorter for you, but let's organize the code a bit.
Would it not be a lot clearer of the code just read:

$currentIP = $_SERVER['REMOTE_ADDR']; // Or check other headers as well.
CheckIfIPExistsInBanIP($currentIP);
$timestamp = time();
$timeout = $timestamp - 900;
DeleteUsers($timeout); // Better call it DeleteObsoleteUers
$res = CheckIfIPExistsInCaspionet($currentIP);
if($res == 0){
InsertUser($timestamp, $currentIP);
}
Fetch Users Online();

As you see, all I have done is taken your own comments from the code and
turned them into functions. If you are reading the code, you are usually
working on or looking for something. This splitting up in functions
allows you to skip all irrelevant parts. Also, functions can be tested.
You can test the banning function by just calling it with an IP that is
in the list and check if it is really as suicidal as it should be. Once
you realize that REMOTE_ADDR is not the only setting that can have an IP
address, this might be very useful.

Also, you free the result after every query. Why not write a function
that performs a query as well? It would allow you to:
- free the results in one place in the code only
- implement error handling
- test the database handling code on a fine-grained level
- it allows you to do any optimizing in just one place (that function)

Furthermore, I would not calculate the time variables in PHP, but in
MySQL. This reduces one query to a constant. If your MySQL version
supports it, you could write stored procedures for the actions. This
sounds like a lot of work, but it should be faster at runtime and it
allows you to restrict database access to execute rights on these
procedures only.

Best regards

AlGorr wrote:
> Hello,,,
>
> I don't know much about PHP+SQL but I have to find the better way to make my
> query work and to be shorter and better.
> All help will be appreciated.
>
> //Check if IP exists in banIP
> $res = mysql_query("SELECT ip FROM banIP WHERE ip =
> '".$_SERVER['REMOTE_ADDR']."'");
> $res = mysql_num_rows($res);
> if($res != 0)
> die;
> mysql_free_result($res);
>
> //Fetch Time
> $timestamp = time();
> $timeout = $timestamp - 900;
> //Delete Users
> $del = mysql_query("DELETE FROM caspionet WHERE timestamp<$timeout");
>
> //Check if IP exists in caspionet
> $res = mysql_query("SELECT ip FROM caspionet WHERE ip =
> '".$_SERVER['REMOTE_ADDR']."'");
> $res = mysql_num_rows($res);
> if($res == 0){
> //Insert User
> $ins = mysql_query("INSERT INTO caspionet (timestamp, ip)
> VALUES('$timestamp','".$_SERVER['REMOTE_ADDR']."')");
> }
> mysql_free_result($res);
>
> //Fetch Users Online
> $res = mysql_query("SELECT DISTINCT ip FROM caspionet");
> while ($row = mysql_fetch_array($res, MYSQL_BOTH)){
> print("IP:$row[0]");
> }
> mysql_free_result($res);
>
> Thank you
> A.K.
>
>

AlGorr wrote:
> Hello,,,
>
> I don't know much about PHP+SQL but I have to find the better way to make my
> query work and to be shorter and better.
> All help will be appreciated.
>

<code snipped>

> Thank you
> A.K.
>
>

I really don't how you can make this much shorter. You need to do all the
queries in it. A few minor points:

First of all, you can have variables in double quotes - you don't need to
concatenate. So, this is valid:

$res = mysql_query("SELECT ip FROM banIP WHERE ip = '$_SERVER['REMOTE_ADDR']'");

You do have a problem with this code:

$res = mysql_num_rows($res);
if($res != 0)
die;
mysql_free_result($res);

You start out by getting the number of rows from the result set ($res). But
then you overwrite the result set. The result is your mysql_free_result will
try to free a number - not the result set itself. Either use a different
variable name (i.e. $numrows) to hold the number of rows returned, or don't use
a variable at all - just use the result from the function, i.e.

if (mysql_num_rows($res) != 0)
die;
mysql_free_result($res);

Also, do you really want to die here? I generally find this to be poor
programming practice. Better would be to redirect them to a "You are banned"
page, i.e.

$numrows = mysql_num_rows($res);
mysql_free_result($res);
if ($numrows == 0) {
header('Location: /banned.html');
exit();
}

Since your only using $timestamp to get a timeout value, forget the extra var.
Just do

$timeout = time() - 900;

Again, you don't need to concatenate a variable within a double-quoted string:


$res = mysql_query(
"SELECT ip FROM caspionet WHERE ip = '$_SERVER['REMOTE_ADDR']'");

And the same problem as above - you're overwriting the result set returned by
the query with the number of rows. So you can't free the result set later. Rather:

$res = ;
if(mysql_num_rows($res) == 0){

Again, no concatenation necessary:

//Insert User
$ins = mysql_query("INSERT INTO caspionet (timestamp, ip)
VALUES('$timestamp','$_SERVER['REMOTE_ADDR']')");
}

The rest looks pretty good.

Just a couple of other things, though. You seem to be trying to track users by
ip address. This is guaranteed to be inaccurate. For instance - you might have
10 users from the same company on your system - all running through the same
proxy server. All will have the same IP address.

Even worse are some companies (like AOL) use a group of proxy servers. Requests
are dynamically routed through the proxy with the least load at the time. This
means every request can come from a different IP address.


IP addresses are not a reliable way to track current users. Cookies are a
little better. You could also use sessions with your own session manager.


--
==================
Remove the "x" from my email address
Jerry Stuckle
JDS Computer Training Corp.
jstucklex@attglobal.net
==================