I need your ideas about my first PHP class

[Soheil]

Hi everybody,

I'm a new one to this community and I'm looking for your help 🙂

I've just written my first PHP class and I really appreciate if you let me know your ideas about it.
Actually I wanna know if I've met the aspects of OOP or not.

This is the code of my class:

(One has to assume that I've connected to the database before initiating an object of this class)

<?php

class Register
{

private $uName;
private $pass;
private $cpass;
private $mpass;
private $email;
private $regTime;
private $membersTable;

public function GetUserData($uName, $pass, $cpass, $email, $regTime)
{
	$this->uName = $uName;
	$this->pass = $pass;
	$this->cpass = $cpass;
	$this->email = $email;
	$this->regTime = $regTime;
}

public function CheckEntries()
{
	if(empty($this->uName))
	{
		return(false);
	}

	if(empty($this->pass))
	{
		return(false);
	}

	if(empty($this->email))
	{
		return(false);
	}

	return(true);
}

public function CheckPassword()
{
	if($this->pass != $this->cpass)
	{
		return(false);
	}

	return(true);
}

public function GetMembersTable($membersTable)
{
	$this->membersTable = $membersTable;
}

public function CheckUserExists()
{
	$sql =  "SELECT uname FROM ". $this->membersTable ." where uname ='$this->uName'";
	$result = mysql_query($sql);
	$num = mysql_numrows($result);

	if ($num > 0) {
		return(true);
	}

	return(false);
}

private function HashPassword()
{
	$this->mpass = md5($this->pass);
}

private function SetRegTime()
{
	$this->regTime = time();
}

public function AddMember()
{
	$sql = "INSERT INTO ". $this->membersTable ." (uname, pass, email, regtime) VALUES ('$this->uName', '$this->mpass', '$this->email', '$this->regTime')";
	$result = mysql_query($sql);

	if(!$result)
	{
		return(false);
	}

	return(true);
}
}

?>

Thanks in advance.

[NogDog]

A few randomly organized thoughts.

• Adding some comments would be useful. They don't have to be verbose, but just enough to give an immediate idea as to what each class member's purpose is. (You might consider using PHPDocumentor comment syntax to facilitate automatic documentation.)

• Some of your "setter" methods use a name beginning with "Get". It would probably be less confusing to call them "Set" instead, to be both consistent and more descriptive.

• I'm not sure how crazy I am about the idea that the table name has to be specified. Shouldn't it always be the same table? If there is some reason to do so, currently nothing forces the client code to set it, so it's possible your insert query will fail due to no table name. Also, since it's name is not known within this class to be a "safe" name, it should probably be quoted (you can use back-tick quotes in MySQL).

• There is no escaping of external inputs being used in the insert query, leaving it open to SQL injection errors/attacks.

• The private methods SetReqTime() and HashPassword() do not appear to be used anywhere, making them redundant for now.

• I generally try to have every (non-constructor) method return something, even if it's just a boolean true at the end, just in case the client code wants to check the return value. (If you don't specify a return value, then it will return false, which may lead client code to erroneously think something went wrong.)

Don't let all those comments get you down (and don't take them as the final word, either -- I am not God's gift to programming by any means). Keep up the OOP efforts and keep learning. 🙂

[Soheil]

Dear NogDog,
It helped me a lot.
I'm gonna try to solve the problems which you mentioned.
But I used those HashPassword() and SetRegTime() for setting the time and making a hashed version of the $pass and then stored them in the database.
Thank you so much.

By the way, for someone like me who is a newbie in OOP programming but also has a relatively solid knowledge about PHP programming, which simple application do you recommend to write with the OOP view?
Actually I was trying to write a membership system.

Thanks.

[NogDog]

Pretty much anything can be done in OOP, so I would simply recommend that you do something that (a) interests you and (b) is not too terribly complex. And even if it's complex, just focus on "objectifying" one specific portion of it for now.

The most important thing is to understand the "why" of OOP, and then the general principles of "how". The specifics of PHP syntax are trivial compared to those concepts. I generally point people to first look at http://java.sun.com/docs/books/tutorial/java/concepts/ for the basic "why" concepts, then dive into the book PHP Objects, Patterns, and Practice (2nd Ed.) for more details and the PHP specifics.

[Soheil]

I'll try to understand concepts deeply.
I appreciate for your comments again.

[NogDog]

Good luck. And almost everyone here generally has had the same experience as me: at first OOP seems strange and full of extra, meaningless code; then one day you have a "eureka moment" where it starts to make sense, and from that point on there's no going back. 🙂

[Soheil]

I see ... 😉

[BuzzPHP]

Hi guys.

I have a very different perspective on this topic. Over the years Ive read though tons and tons of **** code. A lot of which written in OO by people who really didn't know what they were doing.

The fact is, this class offers 2 features. AddMember and CheckUserExists.

In the real world, unless you know what you are doing, you are better off just dealing with well written, highly testing, independant and debuged functions.

But thats my view. I love OOP as much as the next guy, but at the end of the day, you need "bang for buck".

Solid coded functions win over uncooked over engineered OO classes.

below is my view of how this code should have been written (untested).

/**
 ** Adds a new member to the members table.
 **/ 
function AddMember($uname, $pass, $email)
{
  $uname = mysql_real_escape_string($uname);
  $pass = md5($pass);
  $email = mysql_real_escape_string($email);
  $regtime = time();

  return mysql_query("
  INSERT INTO membersTable (uname, pass, email, regtime) 
  VALUES ('$uname', '$pass', '$email', '$regtime')"; 
}
/**
 ** Checks if username exists. Returns true if it does.
 **/ 
function CheckUserExists($uname)
{
  return 
    mysql_numrows(
    mysql_query("SELECT * FROM membersTable where uname ='$uname'")) > 0;
}

[laserlight]

BuzzPHP wrote:

I have a very different perspective on this topic. Over the years Ive read though tons and tons of **** code. A lot of which written in OO by people who really didn't know what they were doing.

The fact is, this class offers 2 features. AddMember and CheckUserExists.

I agree with your assessment. In my opinion, in terms of design, the biggest problem with the original code is that it defines a class that does not model one thing and model it well. The Register class tries to model both a register of members and a single member at the same time.

BuzzPHP wrote:

Solid coded functions win over uncooked over engineered OO classes.

However, your functions are not "solid" because they perform database access with an implicit connection. This makes it more difficult to change those functions to use say the MySQLi or PDO extensions, which would be a motivation for creating those functions (other than the obvious motivation of abstraction and reuse). In fact, because AddMember() returns a resource result rather than a boolean, there is high coupling between AddMember() and the MySQL extension.

[Shrike]

I'll wade in and disagree - the above class is a fair approximation of the Active Record pattern as described in good ol' PoEAA [Fowler]. I would say calling the class 'Register' is inaccurate since it's really about users of some kind. Also how would I get a user from the database - perhaps a find() method of some kind?

Some people may not like the idea of Active Record type classes, but I would not say it's under engineered. It could be better, but that's why we have refactoring. Should it be OOP? That's a more difficult question 😉

[laserlight]

Shrike wrote:

I'll wade in and disagree - the above class is a fair approximation of the Active Record pattern as described in good ol' PoEAA [Fowler]. I would say calling the class 'Register' is inaccurate since it's really about users of some kind.

hmm... okay, I admit that I was misled by Register, $membersTable and GetMembersTable(). It looks like $membersTable is just the table name rather than the rows of the table itself, thus Register does not actually model a register of members. In that case, it is fine in terms of design, although I would declare the various checking functions as private and call them from the constructor instead, throwing an exception if the conditions are not satisfied. However, it should not be "about users of some kind". It should be about a particular user.

[BuzzPHP]

Laserlight, dont confuse "solid" with "upscalable".

Two very different concepts. A lot of clients have no need to support multiple databases and therefore are not willing to pay for that work. As the original code was mysql based, I assumed only mysql would be supported. Support multiple databases in a OO fashion is a totally different ball game. ie: Creating an abstract class and overriding methods to support each DB.

That said, there is nothing stopping someone from patching a function to support a different database.

As far as multiple connections, there is nothing stopping someone from implementing a state system. Very similar to the OpenGL design.

OpenGL supports multiple device contexts even though each function does not include a context object, nor is OpenGL OO in design.

So, my point is. "solid" is very different to "upscalable".

And my overall view is, the less code I have to read to understand a feature, the better. Im not keen on people who code hundreds of lines of code for little in return, especially when Im expected to maintain said code.

ie: No bang for buck.

[Shrike]

BuzzPHP;10930639 wrote:

And my overall view is, the less code I have to read to understand a feature, the better. Im not keen on people who code hundreds of lines of code for little in return, especially when Im expected to maintain said code.

ie: No bang for buck.

This is in fact one of the major benefits of object oriented programming - being able to visualise a program domain in terms of objects, relationships and responsibilities rather than specifically by reading code.

[laserlight]

BuzzPHP wrote:

A lot of clients have no need to support multiple databases and therefore are not willing to pay for that work. As the original code was mysql based, I assumed only mysql would be supported. Support multiple databases in a OO fashion is a totally different ball game. ie: Creating an abstract class and overriding methods to support each DB.

Ah, but there is a difference between only supporting the MySQL extension and only supporting MySQL. By returning a resource result directly, you unnecessarily increase coupling with the MySQL extension. If later the client is informed that a switch to the MySQLi extension would be better on the grounds of better support for newer MySQL features, client code that relies on this (possibly undocumented feature) would break*. Therefore, you should take steps to ensure that there is loose coupling such that the interface can remain stable in the face of such change. It so happens that such loose coupling would allow you to change the database system more easily as well, though obviously not as easily as if you had designed with support for multiple database systems in mind.

BuzzPHP wrote:

That said, there is nothing stopping someone from patching a function to support a different database.

* i.e., there is something stopping someone from patching a function to support a different database, if the client code relies on the fact that the use of the MySQL extension was exposed.

[bubblenut]

Also, even if there were no question of ever needing to change the database or client library decoupling the interface from the implementation is still a good idea; separation of concerns. Regardless of whether the interface is OO, procedural or even command line; it should be well defined and well encapsulated. If my interface is concerned with retrieving, storing and managing users then that is all it should be concerned with. If I save a user I don't care what the result resource is, I just care that it's been saved and that I can retrieve it later. I don't care how or where it's been saved, that may change or may not, the point is, I don't care. And that's somewhere OO makes things a little easier, you are able to define the things that make your interface work (your dependencies) separately from actually interacting with your interface. OO makes it easier for you not to care about the things you don't care about; not a great tag line really is it? 😉

[Weedpacket]

Who's this bubblenut character? 😃