views:

637

answers:

4

I'm not sure which method is best to use when giving READABLE feedback to end user. I've read some forums, but not really gotten any wiser (or I have not understood it)

I would like to give feedback when insertion / update fails, when it's a success and when giving custom feedback (like checking if an item alread exists).

On INSERT, UPDATE, DELETE, DROP etc., the query returns either TRUE or FALSE. Therefor my result property $this->query_result should always be either true or false.

My issues:

  • Dynamically display feedback to user after form submit (submits to same page)
  • $this->query_result is true if it returns a string

I have added code to see what I'm doing (doing wrong)

These are the functions I use for connecting / querying the DB:

  public function connect() 
  { 

      if (!($this->conn = mysql_connect($this->host, $this->username, $this->pwd)))  {
         die("Error connecting to DB by user = " . $this->username); 
      } 

      $this->db = mysql_select_db($this->dbname,$this->conn) 
        or die("Unable to connect to database " . $this->dbname); 
  }  

  private function query($sql) 
  {
      $this->query_result = mysql_query($sql, $this->conn)or die("Unable to query local database <b>". mysql_error()."</b><br>$sql"); 

      if (!$this->query_result){ 
          die("database query failed."); 
      } else { 
          return $this->query_result; 
      }
  }

Here is my problem: I'm giving feedback on the Data Access Layer (DAL), see e.g. this:

  public function addNewPerson($formData)
  {
    $sql = "INSERT INTO my_table(`name`, `email`, `www`)";

    $sql .= " VALUES('".
      $formData['name']."','".
      $formData['email']."','".
      $formData['www']."','");

   $this->query($sql);
   return $this->query_result;
  }

By returning a text string, the return result will always be true. From what I read, I should probably have a function which handles errors / feedback.

This is what I'm currently doing with feedback in my template:

  if (isset($_POST['form_submit']))
  {

    if (isset($_POST['person_submit'])) {
      $formData = $sl->getFormData();
      $result = $myDB->addNewPerson($formData);

      if ($result == true)
      {
        echo '<script type="text/javascript" language="JavaScript">
              jQuery("#contentArea .messageWindow1").show(500);
              jQuery("#contentArea :input").click(function(){ jQuery("#contentArea .messageWindow1").hide(500); });
        </script>';
      } else {
        echo '<script type="text/javascript" language="JavaScript">
              jQuery("#contentArea .messageWindow2").show(500);
              jQuery("#contentArea :input").click(function(){ jQuery("#contentArea .messageWindow2").hide(500); });
        </script>';
      }
    }
  } 

<div id="contentArea">   
  <div class="messageWindow1"> <span class="msg"><?php echo $labelResult ?></span></div>
  <div class="messageWindow2"> <span class="msg"><?php echo $labelResult ?></span></div>
</div>
A: 

You could add a special character to the beginning of the error messages in addNewPerson(). The calling script would use string functions to detect the special character (so that it knows there's an error) and remove that character so that the message would display without it. Do you think this would work for what you're wanting?

DLH
+2  A: 

Just one hint: You should use exceptions when programming OO. For example you could introduce different exceptions for your different error feedback.

class ValidationException extends Exception
{}

class DatabaseExceptionextends Exception
{}

throw ValidationException("Person not saved. Name field was to short or empty.");
throw DatabaseException("database query failed.");

Then you catch all these exception and react differently depending on the exception's type.

try {
    // ...
}
catch (ValidationException $e) {
    // ...
}
catch (DatabaseExceptionextends $e) {
    // ...
}
Philippe Gerber
Thanks Phil. I've only used try/catch in .Net - and basically only the last part of your example. Why do you have the classes? Can't you just throw the error messags from inside the catch? Hmm... I'm a bit unsure how to use this - even though you have a clean and nice example (sorry)
Steven
Steven, he subclasses the standard Exception class mainly so they function as a marker for different kinds of errors. It makes your code more readable and compartmentalized. But there's no use overdoing it since that will just create a mess that no-one can understand.See http://stackoverflow.com/questions/699372/using-the-right-exception-subclass-in-ruby for a related question.
Martijn Heemels
+4  A: 

I would use PHP5's builtin exception handling to trap errors and possibly validation errors too. For ex:

    class DatabaseException extends Exception {}
    class ValidatorException extends Exception {}

         public function connect() 
          { 

              if (!($this->conn = mysql_connect($this->host, $this->username, $this->pwd)))  {
                 throw new DatabaseException("Error connecting to DB by user = " . $this->username); 
              } 

              if(!($this->db = mysql_select_db($this->dbname,$this->conn))) { 
                throw new DatabaseException("Unable to connect to database " . $this->dbname);
 }
          }  

    //....


    public function addNewPerson($formData)
      {
        $sql = "INSERT INTO my_table(`name`, `email`, `www`)";

        $sql .= " VALUES('".
          $formData['name']."','".
          $formData['email']."','".
          $formData['www']."','");

       //If less than 2 characters, do not insert data.
       if (strlen($formData['name']) < 2)
        throw new ValidatorException( "Person not saved. Name field was to short or empty.");

       //If person already exists
       if($this->isPersonInList($formData['name']))
        throw new ValidatorException( "Person already exists!");

       //Process query
       $this->query($sql);
       return $this->query_result;
      }

In the invocation script

try {
$formData = $sl->getFormData();
$result = $myDB->addNewPerson($formData);
} catch (DatabaseException $e) {
// display $e->getMessage()
} catch (ValidatorException $e) {
//display $e->getMessage()
}

Couple of other things to point out with your script.

  1. It's better to use PDO and prepared statements.
  2. You can also use the following to determine if the string length is met.
$arr = 'Shoan';
var_dump(isset($arr[10])); //false
var_dump(isset($arr[2])); //true
  1. Filter the input for sql injection/XSS exploits before pushing it into your database or using it in your application.
Shoan
This was an great example. Thanks! Yeah I know. I will refactor my code when I've put up the basics. I first have to learn PDO and how to use prepared statements in MySQL. I will also Filter input befor sending it to DB. I'm tring to figure outt he best way to do this.
Steven
Oh, one question. What if Insert / delete fails because of constraint issues or duplicate entry (unique key). How will I catch that?
Steven
This is not working :( I get "Parse error: syntax error, unexpected T_THROW". It's the 'throws' in the connect() function.
Steven
I think I fixed it, but haven't verified.
Shoan
Thanks. I will test it. I got another problem. I'm trying to extend my DAL class to use PDO -> "class DAL extends PDO {}".But when reloading the page, it's completely blank - no data what so ever.I'm using PHP v.5.2.6, and the following extensions are enabled: php_pdo, php_pdo_mysql and php_pdo_sqlite.
Steven
I will need to see the code, to be able to help. But if you extend the PDO class, and instantiate an object from the extended class, it should work fine. class DAL extends PDO { }$dbh = new DAL('mysql:dbname=test;host=localhost','root','');I recommend that you read the chapter on OOP in the manual. http://php.net/oop
Shoan
I'm familiar with the basic concepts of OOP. I've added my DAL class below. Once I add the the following code: 'extends PDO', my page goes blank.
Steven
If your page has not output, it would be blank. It is also possible that an error is occurring. Check your server logs.
Shoan
A: 

Have to answer my own example to show code snippet. Taking up on Shoan suggestion, I'm trying to extend DAL to use PDO (class DAL extends PDO). But that gives me blank screen. This is my DAL class.

class DAL { 
  protected $username; 
  protected $pwd; 
  protected $host;
  protected $dbname;
  private $conn; 
  private $db; 
  private $query_result; 

  public function __construct($cfg_file = 'nf.config') 
  { 
    $config = parse_ini_file($cfg_file);

    $this->username     = $config['db_user']; 
    $this->pwd          = $config['db_password'];
    $this->host         = $config['db_host']; 
    $this->dbname       = $config['db_name'];
  } 

  public function connect() 
  { 
      ($this->conn = mysql_connect($this->host, $this->username, $this->pwd))  
        or die("Error connecting to DB by user = " . $this->username); 

      $this->db = mysql_select_db($this->dbname,$this->conn) 
        or die("Unable to connect to database " . $this->dbname); 
  }  


  private function query($sql) 
  {
      $this->query_result = mysql_query($sql, $this->conn)
        or die("Unable to query local database <b>". mysql_error()."</b><br>$sql"); 

      if ($this->query_result){ 
          return $this->query_result; 
      }
  } 

  public function getSomeData()
  {
    $sql ="SELECT * FROM myTable";
    //Process query
    $this->query($sql);
      return $this->query_result; 
  }
}

So in my code, I just do this:
$myDB = new DAL();
$myDB->connect();
$result = $myDB->getSomeData();

But once I add 'extends PDO', my page goes blank. I'm also not able to use any try / catch / throw - it all gives me error messages.

Steven
I get the same result if I try extending to mysqli (class DAL extends mysqli)
Steven
When people suggest using PDO for your queries, they're don't mean you subclass PDO. Subclassing would make sense if your DAL class does the same thing as the PDO class, but slightly different. In your case you simply want to use PDO as a tool, so instantiate it! See http://www.php.net/manual/en/pdo.connections.php and http://www.php.net/manual/en/pdo.prepared-statements.php for an intro.
Martijn Heemels