0

Here is a function in my php file, which is used to serve the request of my android app.

function checkin($DB, $TechID, $ClientID, $SiteID){
    $dbConnection = mysql_connect($DB['server'], $DB['loginName'], $DB['password']);
    if(!$dbConnection){
        die('Error! ' . mysql_error());
        }
    mysql_select_db($DB['database'], $dbConnection);

    $file2 = "C:/wamp/www/file2.txt";
    $data2 = "ClientID:".$ClientID." TechID:".$TechID." SiteID:".$SiteID;
    file_put_contents($file2, $data2);

    $result1 = mysql_query("SELECT COUNT(*) FROM Log") or die('Error! ' . mysql_error());
    $query = "SELECT `Type` FROM `Log` WHERE `TechID` = '".$TechID."' ORDER BY LogTime DESC LIMIT 1";
    $file5 = "C:/wamp/www/file5.txt";
    file_put_contents($file5, $query);
    $result2 = mysql_query($query) or die('Error! ' . mysql_error());
    while($row1 = mysql_fetch_array($result1)){
        $count = $row1['COUNT(*)'];
        $file3 = "C:/wamp/www/file3.txt";
        $data3 = "ClientID:".$ClientID." TechID:".$TechID." SiteID:".$SiteID." Count:".$count;
        file_put_contents($file3, $data3);
        while($row2 = mysql_fetch_array($result2)){
            $file4 = "C:/wamp/www/file4.txt";
            $data3 = "ClientID:".$ClientID." TechID:".$TechID." SiteID:".$SiteID." Count:".$count;
            file_put_contents($file4, $data3);
            /*if($row2['Type']!="Checkin"){
                $count = $count+1;
                $Time = date('Y/m/d H:i');
                mysql_query("INSERT INTO Log (LogID, TechID, ClientID, SiteID, LogTime, Type)
                            VALUES (".$count.", ".$TechID.", ".$ClientID.", ".$SiteID.", ".$Time.", Checkin)");
             }else{
                $query2 = "SELECT TechEmail FROM Tech WHERE TechID=".$TechID;
                $result3 = mysql_query($query2) or die('Error! ' . mysql_error());
                $subject = "Please check out";
                $message = "You have forgot to logout from the last site, please check out manually";
                $from = "devadmin@uniserveit.com";
                $header = "Form:".$from;
                while($row3 = mysql_fetch_array($result3)){
                    mail($row3['TechEmail'], $subject, $message, $header);
                }
            }*/
        }
    }
}

you can see that I have hidden some codes, since I am debugging it, I create some files just to see which part of codes cannot be executed. I discover that the program cannot enter the region where file4 should be created. I have seeked out probably the problem is coming from the $query, when it executes, somethimes the mysql will response "unknown table status: TABLE_TYPE", which I cannot understand why.

Conrad
  • 933
  • 4
  • 19
  • 34
  • 1
    Your function is too large and has too many nestings. Divide and conquer it: Create multiple functions and make the main function call the sub-routines. You can better deal with errors and debugging. And replacing code is much easier, too. – hakre Jul 20 '12 at 08:24
  • It sounds like there might be some duplication in your information_schema. – Kao Jul 20 '12 at 08:26
  • but it should not be the cause of the problem – Conrad Jul 20 '12 at 08:27
  • exactly, when I try to execute the query in phpmyadmin, duplicate errors messages occur – Conrad Jul 20 '12 at 08:29
  • PhpMyAdmin has a bug previous to 3.4 that causes this error. The error is related to PhpMyAdmin, and it's another error in the query that causes your code not to run. – Kao Jul 20 '12 at 08:57
  • 5
    Please, don't use `mysql_*` functions for new code. They are no longer maintained and the community has begun the [deprecation process](http://goo.gl/KJveJ). See the [**red box**](http://goo.gl/GPmFd)? Instead you should learn about [prepared statements](http://goo.gl/vn8zQ) and use either [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli). If you can't decide, [this article](http://goo.gl/3gqF9) will help to choose. If you care to learn, [here is good PDO tutorial](http://goo.gl/vFWnC). – Madara's Ghost Jul 20 '12 at 13:58

2 Answers2

2

As written in the comment above, you should divide and conquer to make your life easier (especially as you write the code while you play around with it in that large function). That does work as easy as:

function file_put($number, $data)
{
    $path = sprintf("C:/temp/wamp/www/file%d.txt", $number);
    file_put_contents($path, $data);
}

for example that is just replacing the many duplicate lines where you just need a (numbered) file you put some string in.

But you can also do this with more complex stuff, like the database operation. You probably want to move the error handling out of your sight as well as taking care to connect to the database when needed and a more flexible way to fetch the data. That can be done by moving the (softly deprecated) mysql_* functions into one or two classes of its' own, so that it gets out of your sight. That will make it's usage much more easier (which I show first):

// Create your database object to use it later on:
$config = array(
    'server' => 'localhost',
    'name' => 'root',
    'password' => '',
    'db' => 'test',
);
$db = new MySql($config);

I called the database class MySql as it represents the mysql connection and it works with the old mysql extension. You only need to pass that database object then into the function in your question. Combined with the file_put function, it would look like this:

function checkin(MySql $DB, $TechID, $ClientID, $SiteID)
{
    $query = sprintf("SELECT `Type` FROM `Log` WHERE `TechID` = '%d' ORDER BY LogTime DESC LIMIT 1", $TechID);
    file_put(5, $query);

    $result1 = $DB->query("SELECT COUNT(*) FROM Log");    
    $result2 = $DB->query($query);

    foreach ($result1 as $row1) {
        list($count) = $row1;
        $data = "ClientID:$ClientID TechID:$TechID SiteID:$SiteID Count:$count"
        file_put(3, $data);
        foreach ($result2 as $row2) {
            file_put(4, $data);
        }
    }
}

Still the checkin function is close to being large (12 lines of code already), but is much shorter than your first version because it delegates the work for writing the files and accessing the database. I hope this demonstration is useful. What follows is the full code example:

/**
 * MySql Exception
 */
class MySqlException extends RuntimeException
{
}

/**
 * MySql Database Class
 */
class MySql
{
    private $server;
    private $name;
    private $password;
    private $db;
    private $connection;

    public function __construct(array $config)
    {
        $this->server = $config['server'];
        $this->name = $config['name'];
        $this->password = $config['password'];
        $this->db = $config['db'];
    }

    private function connect($server, $name, $password)
    {
        $this->connection = mysql_connect($server, $name, $password);
        if (!$this->connection) {
            $this->error("Unable to connect to '%s' as user '%s'", $server, $name);
        }
    }

    private function select($db)
    {
        if (!mysql_select_db($db, $this->connection)) {
            $this->error("Unable to select database '%s'", $db);
        }
    }

    private function close()
    {
        $this->connection && mysql_close($this->connection);
    }

    private function connectSelect()
    {
        $this->connect($this->server, $this->name, $this->password);
        $this->select($this->db);
    }

    /**
     * @param $query
     * @return MySqlResult
     */
    public function query($query)
    {
        $this->connection || $this->connectSelect();
        $result = mysql_query($query, $this->connection);
        if (!$result) {
            $this->error("Unable to execute query '%s'", $query);
        }
        return new MySqlResult($result);
    }

    /**
     * @param string $format
     * @param ...
     * @throws MySqlException
     */
    private function error($format)
    {
        $args = func_get_args();
        array_shift($args);
        $format .= ': %s';
        $args[] = $this->connection ? mysql_error($this->connection) : mysql_error();
        throw new MySqlException(vsprintf($format, $args));
    }

    public function __destruct()
    {
        $this->close();
    }
}

/**
 * MySql Result Set - Array Based
 */
class MySqlResult implements Iterator, Countable
{
    private $result;
    private $index = 0;
    private $current;

    public function __construct($result)
    {
        $this->result = $result;
    }

    public function fetch($result_type = MYSQL_BOTH)
    {
        $this->current = mysql_fetch_array($this->result, $result_type);
        return $this->current;
    }

    /**
     * Return the current element
     * @link http://php.net/manual/en/iterator.current.php
     * @return array
     */
    public function current()
    {
        return $this->current;
    }

    public function next()
    {
        $this->current && $this->fetch();
    }

    /**
     * Return the key of the current element
     * @link http://php.net/manual/en/iterator.key.php
     * @return mixed scalar on success, or null on failure.
     */
    public function key()
    {
        return $this->current ? $this->index : null;
    }

    /**
     * Checks if current position is valid
     * @link http://php.net/manual/en/iterator.valid.php
     * @return boolean The return value will be casted to boolean and then evaluated.
     * Returns true on success or false on failure.
     */
    public function valid()
    {
        return (bool)$this->current;
    }

    /**
     * Rewind the Iterator to the first element
     * @link http://php.net/manual/en/iterator.rewind.php
     * @return void Any returned value is ignored.
     */
    public function rewind()
    {
        $this->fetch();
    }

    /**
     * Count of rows.
     *
     * @link http://php.net/manual/en/countable.count.php
     * @return int The count of rows as an integer.
     */
    public function count()
    {
        return mysql_num_rows($this->result);
    }
}

// Create your database object to use it later on:
$config = array(
    'server' => 'localhost',
    'name' => 'root',
    'password' => '',
    'db' => 'test',
);
$db = new MySql($config);

function file_put($number, $data)
{
    $path = sprintf("C:/temp/wamp/www/file%d.txt", $number);
    file_put_contents($path, $data);
}

function checkin(MySql $DB, $TechID, $ClientID, $SiteID)
{
    $query = sprintf("SELECT `Type` FROM `Log` WHERE `TechID` = '%d' ORDER BY LogTime DESC LIMIT 1", $TechID);
    file_put(5, $query);

    $result1 = $DB->query("SELECT COUNT(*) FROM Log");    
    $result2 = $DB->query($query);

    foreach ($result1 as $row1) {
        list($count) = $row1;
        $data = "ClientID:$ClientID TechID:$TechID SiteID:$SiteID Count:$count";
        file_put(3, $data);
        foreach ($result2 as $row2) {
            file_put(4, $data);
        }
    }
}

checkin($db, 1, 2, 3, 4);
hakre
  • 193,403
  • 52
  • 435
  • 836
  • 1
    Also please see: [How to successfully rewrite old mysql-php code with deprecated mysql_* functions?](http://stackoverflow.com/q/10919277/367456) - Moving those old functions into a database class allows you to replace them at one central location later on while the other part of your codebase already improved. – hakre Jul 20 '12 at 13:48
1

The simple answer should be:

mysql_query("INSERT INTO Log (LogID, TechID, ClientID, SiteID, LogTime, Type)
                        VALUES (".$count.", ".$TechID.", ".$ClientID.", ".$SiteID.", ".$Time.", Checkin)");

remove the quotes from the $vars. or single quote them. Also remove dots. Like:

mysql_query("INSERT INTO Log (LogID, TechID, ClientID, SiteID, LogTime, Type)
                        VALUES ('$count', '$TechID', '$ClientID', '$SiteID', '$Time', Checkin)");
Andreas Nilsson
  • 231
  • 1
  • 6
  • I have successfully tackled the problem, but thank you for providing an alternative solution to me – Conrad Jul 20 '12 at 10:10