Professional Web Applications Themes

How to make it shorter and better? - MySQL

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 = ...

  1. #1

    Default 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.


    AlGorr Guest

  2. #2

    Default Re: How to make it shorter and better?

    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.
    >
    >
    Dikkie Dik Guest

  3. #3

    Default Re: How to make it shorter and better?

    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.
    [email]jstucklexattglobal.net[/email]
    ==================
    Jerry Stuckle Guest

Similar Threads

  1. shorter way to write a comparison??
    By Paweł in forum PHP Development
    Replies: 5
    Last Post: May 29th, 08:57 AM
  2. sure shorter way to do this?
    By Jonathan Moss in forum PHP Development
    Replies: 2
    Last Post: January 28th, 06:19 PM
  3. can this one-liner be squeezed any shorter?
    By Jeff 'japhy' Pinyan in forum PERL Miscellaneous
    Replies: 6
    Last Post: July 28th, 07:49 PM
  4. Shorter URL Script
    By Chris in forum PHP Development
    Replies: 0
    Last Post: July 19th, 01:45 PM
  5. shorter way maybe?
    By Jon Kraft in forum PHP Development
    Replies: 2
    Last Post: June 27th, 01:01 PM

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139