Is this good OOP code?

[stefyx]

hi guys,

let me go into detail. I'm into PHP for like a month now, and want to go further with learning OOP, as everyone says it's much better for big projects.

I know the theory, but tried creating a small application and had some problems. Seems like much more work and I don't get how do I choose the objects. I've also read some topics that said you need to make them like intelligent, with properties, make them totally detached from the exterior, make them display no output and I don't really understand.

 

Let's take an example. 

I'm creating a PHP Cars Classifieds website. I'm making the "add car ad" page.

first the page would check if the user is logged in,what user it is, if he has credits to add the ad and then the page would allow him to go further

there he would choose a lot of stuff about the car..etc and add his ad.

 

what objects should I create for this and what properties should I give them? here is how I did it, and please tell me if it's ok.

<?php
include('mysql_connect.php');
class user_checker {
private $userName;
   private $userLogged = FALSE; //if user is logged
   private $userCredit = FALSE; //if user has enough credit
   private $credit = 0; //the credit the user has
   private $adPrice; //how much costs adding an ad
   private $database;
   private $usersTable;
   private $creditField;
   private $userField;
   function __construct($adPrice,$database,$usersTable,$creditField,$userField){
   $this->adPrice = $adPrice;
   $this->database = $database;
   $this->usersTable = $usersTable;
   $this->creditField = $creditField;
   $this->userField = $userField;
   }
   public function checkValidUser() {
   if (isset($_SESSION['user'])) { //we assume that the login script creates a session with the name of the user.
   $this->userLogged = TRUE;
   $this->userName = $_SESSION['user'];
   mysql_select_db($this->database);
   $this->credit = mysql_result(mysql_query("SELECT `$this->creditField` FROM `$this->usersTable` WHERE `$this->userField` = '$this->userName'"),0);  	      if ($this->credit >= $this->adPrice){
      $this->userCredit = TRUE;   
      }
   }
   if ($this->userLogged == FALSE){ 
      return 'falseLOG';
   }else{
	   if ($this->userCredit == FALSE){
		   return 'falseCREDIT';
	   }else{
		   return 'trueCREDIT';
	   }
   }
	   
   }
}

class ad_adder {
private $userState;
function __construct($userState){
	$this->userState = $userState;
}
public function add(){
if ($this->userState == 'trueCREDIT'){
	//show form
}
//if other do y or z
}
}

$u = new user_checker(5,'website','users','credit','user');
$a = new ad_adder($u->checkValidUser());
$a->add(); 
?>

 

(neglect errors, possible logic/security problems as it's just done for an example.)

 

Not really sure why, but I think it is not. If it isn't what is wrong and how I should have done it?

 

//sorry for any english language mistakes,i'm not a native.

[KevinM1]

Right off the bat, a lot of the properties you have in your user_checker class should be a part of each user.  The mechanism that checks the user shouldn't store that kind of information.  Instead, it should check to see if the user is able to perform a certain action based on the user's current state.  If so, continue processing.  If not, abort and display the error.

 

I'm also weary about your idea to pass in database table info directly into the object constructor.  Again, this kind of information should be part of the user, and there should be a membership system in place with your DB structure that can inform the user object where it belongs.

 

So, something like:

class User
{
   private $userName;
   private $userLevel; //new member?  gold member?  admin?
   private $credits;

   public function __construct($name, $level, $credits)
   {
      $this->userName = $name;
      $this->userLevel = $level;
      $this->credits = $credits;
   }

   public function getCredits()
   {
      return $this->credits;
   }
}

 

User info should be pulled from the DB upon login.  If the query succeeds, build the object and log it into sessions

 

class UserCheck
{
   private static $adPrice;

   public static function setAdPrice($price)
   {
      self::$adPrice = $price;
   }

   public static function checkUserCredits($user)
   {
      if($user->getCredits() < self::$adPrice)
      {
         return false;
      }
      else
      {
         return true;
      }
   }
}

 

We use static here because we're not interested in having an actual object, only the methods it provides for checking the user.

 

So, after you user object is created upon a successful login, you'd only need to do something like:

$userState = UserCheck::checkUserCredits($user);

if($userState)
{
   /* continue processing */
}
else
{
   /* error, not enough money to place the ad */
}

 

It might not be 100% inline with your system, but you should get the idea.

[stefyx]

Thank you very much for taking the time to answer me(was actually not sure if someone will  have the patience to read my horrible code  ). I believe I understand it better now.

 

So I'll write it again and hopefully, you'll be kind enough to check again

 

<?php
include('mysql_connect_select_database.php');
// so i'll think it more like reality. There will be 4 objects: user(a guy), userFind(a guy that is paid to go search about user's history in the town(table), as a login system would be a bit more complex), adAdder that adds ads on the website, and userChecker that is like an extension for adAdder as it gives a seal of approval or disapproval if the user can or not add the ad.

class userFind {
private static $creditsTable;
private static $creditsField;
private static $userField;
public static function findCredits($user){
return mysql_result(mysql_query("SELECT `$this->creditsField` FROM `$this->creditsTable` WHERE `$this->userField` = '$user'"),0);
}

class user {
private $name;
private $credits;
public function __construct($name) {
	$this->name = $name;
}
public function setCredits(userFind::findCredits($this->name)){
	$this->credits = $credits;
}
public function getCredits(){
	return $this->credits;
}
}

$idiot = new user('idiot');
$idiot->setCredits();

class userChecker {
private static $adPrice;
public static function setAdPrice($adPrice){
	self::adPrice = $adPrice;
}
public static function checkUser($u){
if ($u->getCredits() >= self::adPrice){
    return TRUE;
}else{
	return FALSE;
}
}
}

class adAdder {
public static function add($user) {
	if (userChecker::checkUser($user)){ //if approved
		//proceed,show the form ...
                 return TRUE;
	}else{
		return FALSE;
	}
}
}

userChecker::setAdPrice(50);

if(adAdder::add($idiot)){
}else{
echo 'You don\'t have enough privilages to add that';
}

?>

 

 

this time I kinda feel it is right. Not really sure if I didn't overly complicate it. Maybe adAdder should just have a method to check and go see if the user is able to do x itself, and that would keep things simpler.

[KevinM1]

You seem to be on the right track.  One thing caught my eye:

 

 

public function setCredits(userFind::findCredits($this->name)){
      $this->credits = $credits;
   }

 

You don't actually have a variable named $credits that you're assigning to, due to the fact that you don't set the findCredits return value to a variable.  Instead, you should do something like:

$idiot = new User('bubba');
$idiotCredits = userFind::findCredits($user->getName());
$idiot->setCredits($idiotCredits);

 

There were some additional minor syntactical errors, but again, you are on the right track.  Something to keep in mind is that OOP deals with objects.  That sounds dumb and obvious, but it's something that many people trying to learn OOP tend to forget.  Objects are supposed to be simple in concept, if not in execution.  They're supposed to represent one concrete thing, and not much else.  Keeping things simple is what fuels the 'magic' of OOP.  It allows the programmer to combine objects at runtime in order to create larger system components.

 

For example, look at what you've broken your system down into.  You now have a User class that can be used in any other part of your system.  Even better, that User class may also be useful in other sites.  You can possibly distill your verification classes into something more universal as well.  Using them in concert doesn't affect their own innate usefulness, either.  User objects aren't damaged or irrevocably changed by passing them into the other classes' methods.

 

So, as a general rule of thumb, it's better to have a host of simple objects that you can pass around and combine with other objects than one really big, complicated object that attempts to do more than one thing well.

[stefyx]

well, that's great

I think I really got it. Just make simple stuff that you can combine and think of everything as it would be real. In adittion use static for things that act more like mechanisms than objects and give each object normal properties it should have (like don't pass the mechanism the properties of the object, only use it to return values and not store anything it doesn't have an effect on the mechanism itself).

 

btw, the missing $credits variable was just a mistake, I understand I just returned a value that couldn't have been accessed later. But I think this would work:

public function setCredits($credits = userFind::findCredits($this->name)){

      $this->credits = $credits;

}

 

 

well, thanks a lot nightslyr. I'll do a bit more practice on OOP and hopefully, I'll get to master it. (ok , that's a bit too much said)