Opinion on some code I wrote

[Darkelve]

Started learning PHP a few weeks ago, I want to ask if anyone can give me some feedback (warnings, pointers, ...) about some code I wrote, with the objective to learn more about functions as well as MySQL.

 

I created three functions, one to create a new table (in an existing DB), one to add a new Record and one to delete an existing record.

 

Next I want to try to use an array as parameters for these functions.

 

<?php

require 'DB.php';

// Connection string
mysql_connect("localhost", "root", "") or die(mysql_error());
/* echo "Connected to MySQL<br />"; */
mysql_select_db("test") or die(mysql_error());
/* echo "Connected to Database"; */


// Create a MySQL table in the selected database
function newTable($tableName)
{
mysql_query("CREATE TABLE IF NOT EXISTS $tableName (
								id INT NOT NULL AUTO_INCREMENT, 
								PRIMARY KEY(id),
								 name VARCHAR(30), 
								 age INT)")
								 or die(mysql_error());  	 		
}		 

// Insert Two values in the corresponding rows in table $tableName
function newRecordTwoVal ($tableName, $recordName1, $recordName2, $recordVal1, $recordVal2) 
{
$myQuery=mysql_query("SELECT * FROM $tableName WHERE $recordName1=$recordVal1 AND $recordName2='$recordVal2';");

$doesItExist= mysql_fetch_array($myQuery);
if ($doesItExist) {
print ('Person already exists');
}
else {
mysql_query("INSERT INTO $tableName
($recordName1, $recordName2) VALUES('$recordVal1', '$recordVal2'); ")
or die(mysql_error());  
echo "Data Inserted!";
}
}

function delRecord ($tableName, $recordName1, $recordName2, $recordVal1, $recordVal2) 
{
$myQuery=mysql_query("DELETE FROM $tableName WHERE $recordName1=$recordVal1 AND $recordName2='$recordVal2';");

}



newTable('Moderators');
newRecordTwoVal ('Moderators', 'age', 'name', '40', 'Jack');
delRecord('Moderators', 'age', 'name', '40', 'Jack');


?>

[ignace]

1. use built-in functions as intended:

 

$db_res = mysql_connect('localhost', 'root');

mysql_select_db('test', $db_res);

 

2. do not use die() use trigger_error() instead

3. do not create your tables in your application

4. ALWAYS properly escape your data:

 

$tableName = mysql_real_escape_string($tableName);

 

5. use meaningful function names

6. use meaningful argument names

7. do not output inside functions, rather use the returned value and output it afterwards.

8. put related functions in an object

9. only include/require functions (libraries), don't include files that will execute once their included (this helps to avoid #10)

10. don't clutter the global scope

[Darkelve]

Modified the code a bit, hope this is a little better.

 

Still have to work at 7. and 8. of your remarks.

 

<?php
// Connection string
$database_resource = mysql_connect('localhost', 'root');
mysql_select_db('test', $database_resource) or trigger_error('Could not connect to the database');

// Insert Two values in the corresponding rows in table $tableName
function insertNewRecord ($tableName, $recordName1, $recordName2, $recordVal1, $recordVal2) 
{
$selectPerson=mysql_query("SELECT * FROM $tableName WHERE $recordName1=$recordVal1 AND $recordName2='$recordVal2';");

$doesPersonExist=mysql_fetch_array($selectPerson);
// check if person already exists
if ($doesPersonExist) {
print ('Person already exists');
}

// if he does not yet exist, insert the record into the table 
else {
mysql_query("INSERT INTO $tableName
($recordName1, $recordName2) VALUES('$recordVal1', '$recordVal2'); ")
or trigger_error('Could not insert record into table');  	
}
}

function deleteRecord ($tableName, $recordName1, $recordName2, $recordVal1, $recordVal2) 
{
$myQuery=mysql_query("DELETE FROM $tableName WHERE $recordName1=$recordVal1 AND $recordName2='$recordVal2';") or trigger_error('Could not delete record from table');  
}

insertNewRecord ('Moderators', 'age', 'name', '40', 'Jack');
deleteRecord ('Moderators', 'age', 'name', '40', 'Jack');
?>

[Darkelve]

7. do not output inside functions, rather use the returned value and output it afterwards.

 

How can I do that with the SQL statements? If I return the string containing the SQL query, how can I actually execute this query string?

[Darkelve]

I simplified the code a bit for the purpose of learning more about functions and return values (and to keep things clear). I do have a problem though: the SQL statement gets returned, but when I try to execute the query (through mysql_query command), I can see in PhpMyAdmin that the age field does not get updated. Can't see a reason though...

 

<?php

// 1. create connection
$database_resource = mysql_connect('localhost', 'root');
mysql_select_db('test', $database_resource) or trigger_error('Could not connect to the database');


// 2. function - update age of person X in table $tableName
function updateAge ($tableName, $person) 
{
$selectPersonString=("UPDATE $tableName SET age = 60 WHERE name = '$person'");
$selectPersonSql=mysql_real_escape_string($selectPersonString);
return $selectPersonSql;
}

print(updateAge ('Moderators', 'Bruce'));
$newAge=mysql_query(updateAge ('Moderators', 'Bruce'));

?>

[ignace]

Do not make it too difficult, too complex and unmaintainable:

 

updateAge('forum_posts', 'foo'); // doesn't make sense but is currently possible in your design

 

$db = mysql_connect('localhost', 'root');
if($db === false) {
    trigger_error('Failed to connect to the database server.');
}

if(!mysql_select_db('test', $db)) {
    trigger_error('Database test does not exist or could not be accessed.');
}

function person_create($data, $db) {
    if(!person_is_valid($data)) {
        trigger_error('Invalid user data. User has not been created.');
    }
    $sql = 'INSERT INTO users (user_name, user_pass) ' .
           'VALUES (' . db_input($data['user_name'], $db) . ',' .
                        db_input($data['user_pass'], $db) . ')';
    mysql_query($sql);
    return mysql_insert_id();
}

function person_is_valid($data) {
    $data = array_merge(
        array('user_name' => '', 'user_pass' => ''),
        $data
    );
    return !empty($data['user_name']) && !empty($data['user_pass']);
}

function db_input($input, $db) {
    return "'" . mysql_real_escape_string($input, $db) . "'";
}

[Darkelve]

In the code below, I do not understand why my query executes when I use mysql_query with the literal values, but NOT when I use mysql_query with the variable.

 

(disclaimer: I know the code below is far from perfect, but the purpose is to get some practice and get my head around some of PHP's concepts).

 

 

<?php

// 1. create connection
$db = mysql_connect('localhost', 'root');
if($db === false) {
    trigger_error('Failed to connect to the database server.');
}

if(!mysql_select_db('test', $db)) {
    trigger_error('Database test does not exist or could not be accessed.');
}

// 2. function - create new person in table moderators

function createPerson ()
{
$newPerson="INSERT INTO moderators (name, age) VALUES('Mark', '20')";
$sql= "'" . mysql_real_escape_string($newPerson) . "'";
return $sql;
}

$sqlStatement=createPerson();
print $sqlStatement; // seems like the SQL statement is returned OK 
mysql_query($sqlStatement, $db); // this does NOT work
mysql_query('INSERT INTO moderators (name, age) VALUES(\'Mark\', \'20\')', $db);   // this DOES work
?>

[Darkelve]

I added an error check, and the Error message says:

 

Notice: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '\'Mark\', \'20\')' at line 1 in C:\programs\xampp-win32-1.7.3\xampp\htdocs\db-simplified.php on line 25

 

What is the correct way to make use of it?

[Darkelve]

Well, it does work if I change the following rule and strip out the slashes:

 

$sqlStatement=stripslashes(createPerson());

 

However, I wonder if that defeats the purpose of using the mysql_real_escape_string function?

[Darkelve]

Studied a bit more in the meantime... watched some PHP tutorials videos and wrote the following small program. It displays in input box and two radio buttons to select the action to perform. The user enters something in the box and selects a radio button. When the user submits, the corresponding function (SQL select, SQL update) is performed.

 

First I followed the tutorial literally. Then I made the blocks of code into functions. Then I added The SWITCH statement. Finally, I tried using the return value in the 'update' function. To my own amazement, it works.. Tried adding mysql_real_escape_string, but faile (not sure how that works exactly).

 

Any suggestions, comments, criticism are appreciated!

 

 

<?php

$userQuery=$_POST['name'];

$userAction = $_POST['action'];

$mysql = new mysqli('localhost', 'root', 'password', 'db') or trigger_error('Connection to database failed');

switch ($userAction)
{
case 'select':
  selectName($userQuery);
  break;
case 'update':  
  $updateName = $mysql->query(updateName($userQuery)) or trigger_error('Query failed');
  break;
default:
  echo 'No action selected';
} 


/* function to UPDATE the table */
function updateName($userQuery) {    
    $mysql = new mysqli('localhost', 'root', 'password', 'db') or trigger_error('Connection to database failed');
//    $userQuery=$_POST['name'];      
    $updateNameSql= "UPDATE mytable SET names = '$userQuery' WHERE id='1'";
    
    return $updateNameSql;
}

/* function to INSERT into table */
function selectName($userQuery) {

// $userQuery=$_POST['name'];
$getNameSql= "SELECT * FROM mytable WHERE names='$userQuery'";
$getName = $mysql->query("$getNameSql") or trigger_error('Query failed');

if ($getName) {
        while ($row = $getName->fetch_object() )
        $name = $row->names;        
        
        if ($name) {
        echo $name . ' exists in the table';
        }
    }

}
?>

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
  <title>Mysql</title>
</head>
<body>

<div id="content">

    <form method="POST" action="">
            <label for="name">Name: </label><input type="text" name="name" id="name" />
            <br /><br />
            <input type="radio" name="action" value="select" checked="checked" />Select person    
            <input type="radio" name="action" value="update" />Update person ID=1
            <input type="submit" name="submit" value="submit" />
            
    </form>

</div>

</body>
</html>

[Darkelve]

Cleaned it up some more. And for the second function, also re-wrote the code for the other function to use a return value.

 

Next thing to do: write some code to check that the text field is not empty. Because right now, entering a blank value and picking 'update' will make the name field blank.

 

<?php

/* Retrieve the 'name' and 'action' values from the FORM */
$userQuery=$_POST['name'];
$userAction = $_POST['action'];

/* Set up a new Mysql instance: conntection to database 'db' */
$mysql = new mysqli('localhost', 'root', 'password', 'db') or trigger_error('Connection to database failed');

/* Execute a function, based on the selected radiobutton */
switch ($userAction)
{
// Execute the selectName Function
case 'select':
  $getName = $mysql->query(selectName($userQuery)) or trigger_error('Query failed');
	if ($getName) {
			while ($row = $getName->fetch_object() )
			$name = $row->name;        

			if ($name) {
			echo $name . ' exists in the table';
			}
		}; 
	break;
  
  
// Execute the updateName Function
case 'update':  
  $updateName = $mysql->query(updateName($userQuery)) or trigger_error('Query failed');
  break;

  // Code to execute if no radiobuttons are selected
  default:
  echo 'Please select an action';
} 


/* function to UPDATE the table */
function updateName($userQuery) {    
    $updateNameSql= "UPDATE mytable SET name = '$userQuery' WHERE id='1'";
    return $updateNameSql;
}

/* function to SELECT from the table */
function selectName($userQuery) {
$getNameSql= "SELECT * FROM mytable WHERE name='$userQuery'";
return $getNameSql;
}
?>

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
  <title>Mysql</title>
</head>
<body>

<div id="content">
    <form method="POST" action="">
            <label for="name">Name: </label><input type="text" name="name" id="name" />
            <br /><br />
            <input type="radio" name="action" value="select" checked="checked" />Select person    
            <input type="radio" name="action" value="update" />Update person ID=1
            <input type="submit" name="submit" value="submit" />
    </form>
</div>

</body>
</html>

[Yesideez]

Just a quick note.

 

If your web space (or future web space) has PHP set up to be strict this:

$userQuery=$_POST['name'];

will throw a warning. I'd use a ternary here like this:

$userQuery=(isset($_POST['name']) ? $_POST['name'] : '');

That way if the $_POST['name'] is set it'll get the content and assign it to the variable otherwise it'll be assigned an empty string. You could replace '' with null for example:

$userQuery=(isset($_POST['name']) ? $_POST['name'] : null);

Or instead of null you could even specify a default value.

[Darkelve]

Thanks!

 

I've been thinking... what's the most clean/efficient way to use functions?

 

For example, in the post above, I used the functions with return values... but

- is this really the best way to do it?

- wouldn't it be better to execute the query inside the function, so that it is more like a 'complete' block of code?

- OR  should I make a separate function where the DB connection is created and the SQL queries executed?

- OR ... something else?