Love some feedback...

[Runnion]

This code reads from a Windows Media Server log directory and parses the log files. It then writes the fields selected to an SQL database so we can crunch the numbers and send out ENORMOUS invoices.

Thanks for the feedback!

It's attached and here.....

PHP Code:

<div style="font-size: 11px; font-family: arial;">
<?php
error_reporting (E_ALL); // turn on all error reporting
$id = 0; // a unique id for the $record field;
$record = array(); // where we keep our array of fields

function GetLogs($host, $username, $password)
{
    $error = "Unable to get the log files from the media server via FTP";
    $logs = array(); // array of all the log files we want
    
    $conn_id = ftp_connect($host); // open a stream to the ftp host
    $login_result = ftp_login($conn_id, $username, $password); // login to the ftp host
    
    if((!$conn_id) || (!$login_result)) // If we weren't able to connect to the ftp server
    {
        die($error." 1");
    }
    $dir = ftp_pwd($conn_id) or die($error." 2"); // get the name of the current directory
    $logs = ftp_nlist($conn_id, $dir) or die($error." 3"); // read all the files in the directory into an array
    
    foreach($logs as $key=>$value) // loop through our array of files and get each one.
    {
        $local_file = substr($value, 8, 9).".log"; // change the names of the logfiles for easier use.
        echo strtoupper($value. " downloaded as $local_file<br>"); // report successful downloads
        ftp_get($conn_id,$local_file,$value, FTP_BINARY) or die($error." 4");
    }
    ftp_close($conn_id);
    echo "<br><br>";
}

function FileHandler($dir, $id, $record)
{
    $ignore = array("index.php", "********", ".", ".."); // these are the files in the download directory that we want to ignore
    $error = "Unable to work with the downloaded log files"; // our error message
    
    if(!is_dir($dir)) // make sure we have an existing directory.
    {
        die($error. " 1");
    }    
    if(!$dir_handle = opendir($dir)) // make sure we can open the directory.
    {
        die($error. " 2");
    }
    while($file = readdir($dir_handle)) // loop through all the files
    {
        $found = false; // to keep track of whether we've found a match to our ignore array
        foreach($ignore as $value)// loop through our array and see if we find a match to our ignore list
        {
            if($value == $file)
            {
                $found = true;    
            }
        }
        if(!$found) // this isn't an ignored file so parse it.
        {
            FileParser($file, &$id, &$record); // pass by referrence (&$) to our FileParser
        }
    }
}

function FileParser($file, $id, $record)
{
    $error = "Unable to open $file for parsing";
    $first_line = 4; // first line of the log we want to parse, this gets rid of the headers
    $delimeter = " "; // the text delimeter of the log
    
    $client_ip = 0; // field numbers in the order from the logfile
    $date = 1;
    $time = 2;
    $client_requested_file = 4;
    $duration = 6;    
    $client_useragent = 12;
    
    if(!$file_array = file($file)) // make sure we can open the file into an array
    {
        die($error);
    }
    foreach($file_array as $key=>$value) // loop through the array and perform this code to each line
    {
        if($key < $first_line) // we haven't reached the first line we want yet, so we break out of the loop
        {
            continue;
        }
        else
        {
            $id ++;
            $fields = explode($delimeter, $value); // break apart this line into a mini array by looking for spaces
        
            $record[$id] = array($fields[$client_ip],  $fields[$duration], $fields[$date], $fields[$time] , $fields[$client_requested_file], $fields[$client_useragent]);            // write the record to a record array
        }
    }

}

function ShowResults($record) // This is only for debugging purposes.  It writes all the record lines to the browser
{
    foreach($record as $key=>$value) // for each unique record
    {
        echo "record $key=>    ";
        foreach($value as $subkey=>$subvalue) // for each field of each record
        {
            echo $subvalue."      ";
        }
        echo "<br>\n";
    }
}

function WriteToDatabase($record)
{
// this function maintains our database connection
    $username = "********"; // our database login variables
    $password = "********";
    $db = "*********"; // the database to use
    $host = "*********"; // the database server
    $conn_id = mssql_connect($host,$username,$password); // connect to the database server
    $db_id = mssql_select_db($db, $conn_id); // select the database    
    
    $table_name = date("FY")."Stats";
    
    $query = "select id from $table_name"; // here we're checking to see if this month's table has been made yet
    if($result = @mssql_query($query)) // the @ sign suppresses the error message we'd get normally get when the query failed
    {
        // table exists
        $num_rows = mssql_num_rows($result); // how many records have already been put in the database?
        WriteQueries($record, $num_rows, $table_name); // forward that number to the query writer
    }
    else
    {
        // table doesn't exist.  create it.
        $query = "CREATE TABLE $table_name
            (
            id INTEGER NOT NULL PRIMARY KEY IDENTITY(1,1) ,
            client_ip varchar (16),
            duration INTEGER,
            date varchar (15),
            time varchar (20),
            client_requested_file varchar (80),
            client_useragent varchar (150),
            )";
        $result = mssql_query($query);
        $num_rows = false; // there are no records in our brand new table
        WriteQueries($record, $num_rows, $table_name); // pass that information to the query writer
    }
}

function WriteQueries($record, $num_rows, $table_name)
{
// this function writes and runs all our insert queries
    if($num_rows === false) // the triple = sign means that the variable has to match exactly, including data type.  this way the number 0 won't match for false in the case of an empty table.
    {
        $counter = 1; // set the counter to 1 because we're putting in all new records
    }
    else
    {
        $counter = $num_rows+1; // set the counter to the first row after the ones in the database
    }
    for($counter; $counter <= count($record); $counter++) // run through the loop from wherever the counter starts so we don't ever enter duplicates
    {
        foreach($record[$counter] as $subkey=>$subvalue) // loop through ever field in each record
        {
            $temp_array[$subkey] = $subvalue; // use a temporary array for this record
        }
        
        $query     = "insert into $table_name (client_ip, duration, date, time, client_requested_file, client_useragent) ";
        $query .= "values(\"".$temp_array[0]."\", \"".$temp_array[1]."\", \"".strtotime($temp_array[2])."\", \"".$temp_array[3]."\", \"".$temp_array[4]."\",\"".$temp_array[5]."\")";
        
        $result = mssql_query($query); // insert the record
        
    }
}

GetLogs("*******","*********","**********");
FileHandler("***********", &$id, &$record);
// used for debugging ShowResults($record);
WriteToDatabase($record);

?>
</div>

[BuzzLY]

To make it easier to read, wrap some of your lines. That's one of the great things about PHP... as long as a line doesn't end with a semi-colon (;), for the most part, it assumes the line is continued on the next line.

You can start by putting your comments before each code line, instead of in the same line.

Not required for programming, of course -- it just makes your code much easier to read.

[Jeb.]

Expanding on Buzz's commenting comment (I know, that was shameless ), I think you've gone overkill on the commenting.

You don't need to explain what every single line of code does, just the important bits. Comment your functions, explain what they do at the top of the function (at least, that's what I do), and maybe comment a few really complex blocks of code within them.

But don't do a comment for every line. That will actually make the script much harder to read in future.

Now, another note. In your FileHandler function, you're looping through the $ignore array for every file in the directory, and checking manually.

Not necessary, and that will add up to a lot of iterations (especially if you add new disallowed files). Simply use in_array , and PHP will take care of all that for you.

Also, to skip those 4 header lines in FileParser, you can also use something like array_splice or even just 4 array_shift calls, instead of wasting those 4 loop iterations. It'll also make things easier to understand, and remove a few lines of code. But, your call!

That's as far as I got. It's very well set out and well written. Nice work

[Weedpacket]

Originally posted by Jeb.
You don't need to explain what every single line of code does, just the important bits. Comment your functions, explain what they do at the top of the function (at least, that's what I do), and maybe comment a few really complex blocks of code within them.

But don't do a comment for every line. That will actually make the script much harder to read in future.

Coincidentally, while I read the above I had in another window a script I'm tweaking. The file is 140 lines long, but the first hundred lines is commentary about what the other forty lines are intended do, why they exist, and how to use them. And to use them takes two lines.

[Runnion]

That's great feedback! Thanks folks.