more oop critique please!

jonno946

Hi there,

in an attempt to practise my PHP 5 oop skills, i have made a user authentification class that logs users in, out, and also resends forgotten passwords and remonds them of forgotten usernames.

this all works fine....

can any1 critique the code for me?

next i want to add a register feaure and i have one quesion about this...

i want to make this script a framework for all of my websites, and obviously each website will require different registration questions...

how can i make the registration function reusable, in the sense that i dont have to play around with large chunks of of the class code for each website?

i suppose i could make a validation class and then just define the variable name, mysql column name and validation rules for each question in an array at the top of the page?

anyway my code is attached 🙂

thanks!

bubblenut

That looks really good. One thing which imediately stands out by it's absense is the use of PHP5's new exception handling model. This would make bubbling errors up through your inheritance heirarchy much more manageable. Related to this point is the fact that you're handling the possibility of database errors. Below is an example of how you might.

protected function updateDBTime($column) {
  if(!@mysql_query(...)) {
    throw new Exception('The query failed');
  } else {
    return true;
  }
}

Or you could go one step further and create your own custom exception for database exceptions (there's an example of doing this is the manual section on exceptions).

After looking back through it I'm not sure of the valididty of your inheritance. A good rule of thumb that I use is the kind of/part of question. Now, is user_management a kind of basics or is basics a part of user_management? I would be inclined to go with the later, in which case an object of type basics should be used within the user_management class rather than extending the basics class. You would, of course, loose the protection of the functions.

With regards to your specific question about making a reusable registration method, I would say don't define the columns that are to be used. I personally like associative arrays and would probably pass the method one with column names as keys and the appropriate values as values, prehapse with some meta data so that we know how to validate them. Another, arguably more appropriate, way would be to have an array of objects or even an iterator object.

Hope this sparks off some ideas (...other than "That Bubblenet is such a twat" 😉)

jonno946

thanks for your feedback bubblenut!, i think your right about the lack exception handling bit...

i'm going to have a look into it as i've not had the chance to play and experiment with exceptions as yet!

infact, maybe you can tell me what are the advantages of using PHP5's exception handling capabilities are, against checking for functions returning FALSE like i have done?

i also think your right about the user_management extendin basics, i'm going to have a play later and make basics an object instead of inheriting it.

thanks again!

bubblenut

It makes exception handling a lot cleaner. Firstly it's much simpler to bubble errors up through your classes, for example:

class a {
  function methodA() {
    throw new Exception('The Error');
  }
}

class b {
  function methodB()  {
    $a=new a();
    $a->methodA();
  }
}

$b=new b();
try {
  $b->methodB();
} catch(Exception $e) {
  echo($e);
}

Notice that the exception is not caught and then re-thrown in class b, it is allowed to bubble up and is then only cought at when the error is going to cause an issue.

This kind of exception handling really starts to shine when you start using (and creating) different Exception types for different errors. For example:
Assume that the exception classes are already written. Also, this example is quite OTT, you wouldn't need all those different exceptions for such a simple task.

function writeToFile($file, $data) {
  if(!file_exists($file)) throw new FileNotFoundException();
  if(!is_writable($file)) throw new FileNotWritableException();
  if(empty($data)) throw new EmptyDataException();
  if(!$fp=fopen($file, 'w')) throw new FailedOpenException();
  if(!fwrite($fp, $data)) throw new FailedWriteException();
  return true;
}

try {
  writeToFile($file, $data);
} catch(FileNotFoundException $e) {
  //handle exception
} catch(FileNotWritableException $e) {
  //handle exception
} catch(EmptyDataException $e) {
  //handle exception
} catch(FailedOpenException $e) {
  //handle exception
} catch(FailedWriteException $e) {
  //handle exception
}

See? You can apply differnt code depending on the nature of the exception. Some may be recoverable from, others may be fatal.

kpzani

I haven't read all of your code!

but i have done a very similar thing.

I use eval to dynamically run things like functions so that each page or instance can run a different function.

my use is as follows
I have a class for a database access page which is used like this
$mypage = new iscaPage("title");
$mypage->DTables = array("tbl1", "tbl2");
$mypage->Fields = array("tbl1.field1", "tbl2.field2");
$mypage->NewFunction = "CreateNewStuff();";

in the class if the NewFunction is not null then it evals it
eval($this->NewFunction);
so as long as the function is defined in this script or an include then it'll run.

Its an odd blend of oop and functional programming but it seems to work really well for me - and therefore maybe you too!