What do you think to my Database class?

[johnsmith153]

It is designed so you can dump it in a PHP file and open in your browser with no need for actual db access.

 

Some people say to only use 2 classes, not 3 deep. What do you think?

 

<?php

//simulate $_POST
$_POST['name'] = "Dave Smith extra long name here";
$_POST['username'] = "smith123";

$_POST['searchname'] = "Dave";
$_POST['searchUserID'] = "555";


class Database {

//plus lots of other stuff (basically everything else not already shown below)

protected function escape($val) {

	//mysql_real_escape_string
	return $val;

}

protected function query($sql) {

	//mysql_query
	return $sql;

}

}

class DatabaseControl extends Database {

public $connection;
public $errors = array();

public function __construct($db) {
	// grab db connection variable
	$this->connection = $db;
}

//logs errors when preparing to add/update record
public function addError($num, $text) {

	$this->errors[$num] = $text;

}

public function go($class, $method) {

	call_user_func(array($class, $method), $this);

}

public function checkMaxCharacters($num, $text, $val) {

	if(strlen($val)>20) {

		$this->addError($num, $text);

	} else {

		return $val;

	}

}

}

class User extends DatabaseControl {

public static function findUser() {

	$name = parent::escape($_POST['searchname']);
	$userID = parent::escape($_POST['searchUserID']);
	$sql = "SELECT Name FROM users WHERE `Name` = '{$name}' && UserID = '{$userID}'";
	echo "THIS WAS COMPLETED: " . parent::query($sql);

}

public static function addUser($object) {

	//check input
	$nameEntered = $object->checkMaxCharacters(1, "Name has too many characters", $_POST['name']);
	$usernameRequested = $object->checkMaxCharacters(2, "Username has too many characters", $_POST['username']);

	$errors = $object->errors;

	if(count($errors)>0) {

		//errors so don't add
		foreach($errors as $v) {

			echo $v . "<br />";

		}

	} else {

		//good to go
		$nameEntered = parent::escape($nameEntered);
		$usernameRequested = parent::escape($usernameRequested);
		$sql = "INSERT INTO users (`name`, `username`) VALUES ('{$nameEntered}', '{$usernameRequested}')";
		echo "THIS WAS COMPLETED: " . parent::query($sql);

	}

}

}

$db = new Database();

$getUser = new DatabaseControl($db);
$getUser -> go("User", "findUser");

echo "<br /><br />";

$addUser = new DatabaseControl($db);
$addUser -> go("User", "addUser");

?>

[trq]

The first major floor I see is the use of the $_POST array within your methods. This ties your code to the $_POST array, making it un-reusable.

 

The second is that a User would never extend a database. The two are unrelated.

[johnsmith153]

To be honest $_POST was for testing. I will eventually build a class that captures POST and GET variables and other address bar values (as I use mod_rewrite). So ignore this. I nearly made a note in the script but thought somebody would pick up on the note I wrote!

 

Can you explain about "a User would never extend a database"?

 

So, overall are you saying I should scrap this?

[trq]

Can you explain about "a User would never extend a database"?

 

Why would a User extend a database? They are not at all related. A User might need a database to be able to get data from it, but a User is not a type of database. It makes no sense.

 

So, overall are you saying I should scrap this?

 

There isn't anything particularly good about the design so.....

[johnsmith153]

Could you provide some advice how to improve?

 

The things I like about the design are:

 

(i) I can use multiple databases with it.

(ii) I can use the DatabaseControl class to do things like check user input (max characters etc.) and also control errors when dealing with user input for add/update scripts.

(iii) Perfect code-reuse. Nothing is duplicated.

(iv) Tiny controller code (outside of the class).

 

I would be surprised if you could find a better script that is as simple as this that does the four things above that I want.

 

I'm no expert btw, and to challenge you is rather crazy as you must have 100 times the knowledge I have, but I do think this is better than you may be giving credit - or at least there is a 5/10 minute fix to gain your approval.

 

Why would a User extend a database? They are not at all related. A User might need a database to be able to get data from it, but a User is not a type of database. It makes no sense.

That sounds like there is an easy fix (?)

[johnsmith153]

Would this improve it?

 

(1) remove the fact that DatabaseControl extends Database.

 

(2) change 'parent' to 'Database'

[trq]

(i) I can use multiple databases with it.

 

Where? It does not provide any abstraction.

(ii) I can use the DatabaseControl class to do things like check user input (max characters etc.) and also control errors when dealing with user input for add/update scripts.

 

Validation has nothing to do with a database, and simply echoing the error (html and all) from within your objects provides no flexibility whatsoever. Your checkMaxCharacters() method provides NO way of setting what the max chars are.

(iii) Perfect code-reuse. Nothing is duplicated.

 

Perfect code re-use? Nothing is duplicated now maybe, but most of this code would need to be rewritten before it would be re-usable.

(iv) Tiny controller code (outside of the class).

 

This does what?

 

I can understand that you yourself are likely pretty proud of your effort and you should be. It doesn't necessarily mean its well designed though.

[johnsmith153]

My Database class is purely an example and very scaled down. The SQL queries are just examples.

 

(i) The idea is to use

$db1 = new Database(1, "F");//first append the database number and "B"asic or "F"ull account permissions
$db2 = new Database(2, "F");

...only as needed of course

 

I know this isn't in the code, but I'm more interestes in other parts (hence why Database has no detail in it).

 

(ii) This is enough for me (I just want an array of error codes). As for setting the value - again this is purely an example. If I've been passing objects in and out of functions I'm hardly going to have a set value for the max characters (there will be of course many more checks as well).

 

I think the fact that I put the max characters function there is very good. Where else would I put it (Database / Users)?

 

(iii) Again, testing only - purely an example.

 

Admit in the example the findUser and addUser are very specific, and I could work on this, but this would be the same if I got a perfect Database class and added these bits to it. I don't think this makes my class poor.

 

(iv) I personally prefer to see what I've used in the controller code / outside of the class, than something like this:

$db = new Database(); //Creating new object
$db->init("localhost","test_root","test_pwd!","test_db"); //initializing by credentials.
$db->connect(); //unicode support
$test_value = $db->siftDown($test_value); //preventing harmful inputs
$db->query("SELECT * FROM test_table WHERE test_field = '$test_value' AND second_test_field = '$something_testy_else' LIMIT 1");
if($db->countRows()==1)
$dbdata = $db->loadRows(); //returns a numeric/associative array as the result (MYSQL_BOTH)
$db->disconnect();

 

Could you provide some advice how to improve?

[johnsmith153]

To be honest, my script isn't really database related. I haven't even provided any database code (the Database class is empty).

 

I just want to know if this is the right way to 'prepare' data for a database (I think this explains it better).

[johnsmith153]

Whilst my class may be poor, I'm asking on here because I don't know, I don't feel your answers fit the qustion I was asking.

 

OK, I said "What do you think of my Database class?" - but the fact that the actual 'Database' class (one of three classes) is empty should make things obvious.

 

I also asked "Some people say to only use 2 classes, not 3 deep. What do you think?" - which I would really like an answer on.

 

I am still trying to get an answer on how I can improve. I'm sure there is, but I don't see constructive comments in here how to improve, just a clear indication I should scrap it (you didn't say directly).

[trq]

I suppose I wasn't very constructive, where to start? The db class is pretty well empty, I'll just ignore that. There are plenty of database abstraction classes around so I would bother writing your own.

 

The DatabaseControl object to me looks like it should be split into Validation and Error Handling of sorts. You should take a look at how exceptions work within php.

 

As for the User class, it is not, and should not be a type of Database (which is what happens when you extend a base class). The methods findUser() and addUser() also do not belong to a User class, they belong in a UserManager class or similar. Its not a User's job to add more Users.

 

Does this help at all?