Use class properties or pass data around as method arguments

[nighthawk]

I know basics of OOP and I use it for every project including more than about 100 lines of code. However I have a nagging question.

If I have a class that accepts some data through its public method(s) and then does something with/to that data with protected/private methods, is it better to use class properties, or pass data around as method arguments? For example:

Code: Select all

class Example{
  private $_foo;
  private $_bar;

  public function insert($foo,$bar){
    $this->_foo = $foo;
    $this->_bar = $bar;

    if($this->check()){
      //insert into DB or do some other things
    }
  }

  protected function check(){
    //some validation code, return true or false
  }
}

Would it be better instead to pass the data to the check() method as arguments without creating class properties, like this:

Code: Select all

class Example{

  public function insert($foo,$bar){

    if($this->check($foo,$bar)){
      //insert into DB or do some other things
    }
  }

  private function check($foo,$bar){}
}

As far as I can say, not creating additional variables would decrease memory usage slightly (or am I wrong), but on larger projects this kind of code would get too cumbersome, at least for me. On the other hand, for some simpler things, the latter approach seems more elegant because there's less code involved.

So, what can you tell me about advantages and disadvantages of these two approaches.

Thanks in advance.

[requinix]

Forget memory usage. It's just not that important.

In general the first approach is better. It's much easier to work with. With the second the calling code has to track multiple variables that it doesn't necessarily care about, and it's basically just a bunch of glorified functions. Not OOP.

Imagine a more complicated scenario:

Code: Select all

class User {

	public function insert($firstname, $lastname, $email, $address, $city, $state, $zip, $country) {
		//
	}

	public function update($id, $firstname, $lastname, $email, $address, $city, $state, $zip, $country) {
		//
	}

	public function validate($firstname, $lastname, $email, $address, $city, $state, $zip, $country) {
		//
	}

}

Do you really want to have to work with that? An alternative is

Code: Select all

class User {

	private $id;
	private $firstname;
	private $lastname;
	private $email;
	private $address;
	private $city;
	private $state;
	private $zip;
	private $country;

	public function __construct() {
		//
	}

	public function setAddress($address, $city, $state, $zip, $country) {
		$this->address = $address;
		$this->city = $city;
		$this->state = $state;
		$this->zip = $zip;
		$this->country = $country;
	}

	public function setName($firstname, $lastname) {
		$this->firstname = $firstname;
		$this->lastname = $lastname;
	}

	public function save() {
		if ($this->id) {
			// insert
		} else {
			// update
		}
	}

	public function validate() {
		//
	}

}

[Christopher]

So, what can you tell me about advantages and disadvantages of these two approaches.

Both of your approaches are essentially procedural, so neither is better.

As far as I can say, not creating additional variables would decrease memory usage slightly (or am I wrong), but on larger projects this kind of code would get too cumbersome, at least for me. On the other hand, for some simpler things, the latter approach seems more elegant because there's less code involved.

I agree that you should not worry about memory much, except obvious allocation of many vars/elements. In fact, trying to save memory can sometimes cause more memory use -- don't try to outguess the optimizations.

I know basics of OOP and I use it for every project including more than about 100 lines of code. However I have a nagging question.

I get the sense that you are using the OO language constructs, but you are not actually programming OO. One rule to generally follow is that an object should be usable upon instantiation. Caveat - there are a number of good OO practices that do not follow this rule (you will need to learn those), but in general it is the rule to follow.

So this would be preferable:

Code: Select all

class Example
{
  private $_foo;
  private $_bar;

  public function __construct ($foo,$bar)
  {
    $this->_foo = $foo;
    $this->_bar = $bar;
  }

  public function insert()
  {

    if($this->check()){
      //insert into DB or do some other things
    }
  }

  protected function check()
  {
    //some validation code, return true or false
  }
}

Likewise with requinix's code, a next step would be to make it more like a Value Object. Imagine you got this object from your Model when you asked for a User. The Model would fill in the data (perhaps the constructor should take a row array) and then you could change fields. Or you could create a new user object, with no $id, and give it back to the Model to insert.

Code: Select all

class User
{
        private $id;
        private $firstname;
        private $lastname;
        private $email;
        private $address;
        private $city;
        private $state;
        private $zip;
        private $country;

        public function __construct($id, $firstname, $lastname, $address, $city, $state, $zip, $country)
        {
                $this->setId($id);
                $this->setName($firstname, $lastname);
                $this->setAddress($address, $city, $state, $zip, $country);
        }

        public function setId($id)
        {
                $this->id= $id;
        }

        public function setName($firstname, $lastname)
        {
                $this->firstname = $firstname;
                $this->lastname = $lastname;
        }

        public function setAddress($address, $city, $state, $zip, $country)
        {
                $this->address = $address;
                $this->city = $city;
                $this->state = $state;
                $this->zip = $zip;
                $this->country = $country;
        }

        public function save()
        {
                if ($this->id) {
                        // update
                } else {
                        // insert
                }
        }

        public function validate()
        {
                //
        }

}

PS - I'd recommend protected over private, unless you absolutely know that you want to forbid a child class from accessing a property.

[nighthawk]

One rule to generally follow is that an object should be usable upon instantiation.

Thanks for pointing that out. This made me pause and think.

I am using constructors in my code, but probably not in the right way. In a typical setup my classes extend one abstract class that is kept outside the project folder and included into all projects (there is only one copy of this class on my server, in other words). The abstract class is responsible for constructing database handle (PDO) and some other things, such as constructing tables and forms from array returned by database query, so I can easily output these things.

Code: Select all

<?php
	namespace nighthawk\projects\Pim;
	
	require(dirname(__FILE__).'/../config/loadParams.php'); //configuration file for this project
	
	require_once($dbcon); //path to the DbCon class
	
	class Users extends \nighthawk\globalClass\DbCon {
		/**
		*	Database handle
		*/
		protected $_db;

		public function __construct(){
			try{
				global $confFile; //configuration file with alternative database parameters
				$this->_db = parent::getDb($confFile); //if this is null DbCon will use the default database parameters
			}
			catch(Exception $e){
				global $dbcon, $debug;
				if($debug){ //development environment
					echo $e->getMessage();
				}
				else{ //production environment
					echo '<p>System failure, sorry for the inconvenience. Please try again later</p>';
				}
			}
		}

                //so, now I would do things like
                public function logIn($username,$password){
                         $salt = $this->getSalt($username);
                         $hash = $this->genHash($salt,$password);
                         //........
                }

                public function registerNewUser($username,$password,$email){}               
?>

IMHO, this is good approach because I can switch databases by editing just a few lines and my loadParams.php file actually sets parameters programatically based on which host it is:

Code: Select all

<?php

	$server = gethostname();
	
	if($server == 'debian2' OR $server == 'www-deb'){ //these are my virtual machines
		/**
		*	Location of global DbCon class
		*/
		$dbcon = '/var/www/global/lib/dbcon.php';
		
		/**
		*	Location of conf file with alternative database parameters, if left blank, default DB will be used
		*/
		$confFile = '/var/www/dev/projects/pim/config/altConf.inc.php';
		
		/**
		*	Sets whether debug information should be shown (development server), or whether generinc error message should be sent (production server)
		*/
		$debug = true;
	}

        if($server == 'public-hosting'){
        //different parameters
        }
	
?>

So, with your suggestion, I should probably make the constructor of my Users class accept $username because all of the public methods will require the username to be set. Then, other methods would accept only specific parameters. Right?

My apologies to anyone who got a headache reading this post.

[Celauran]

Your classes shouldn't have includes in them, and you shouldn't use the global keyword, well, ever really. If your class needs access to something, pass it in. Inject dependencies into your objects, don't pull them in.

[Christopher]

One rule to generally follow is that an object should be usable upon instantiation.

Thanks for pointing that out. This made me pause and think.

Keep pausing and thinking. One thing you should know: software design is not intuitive, in fact it is counter-intuitive, and most programmers need to prove themselves wrong to understand that.

I agree with Celauran, but would have added a bunch of !!!!!!!!!

Code: Select all

<?php
	namespace nighthawk\projects\Pim;
	
	class Users {
		/**
		*	Database handle
		*/
		protected $_db;

		public function __construct($db){
			$this->_db = $db;
		}

                //so, now I would do things like
                public function logIn($username,$password){
                         $salt = $this->getSalt($username);
                         $hash = $this->genHash($salt,$password);
                         //........
                }

                public function registerNewUser($username,$password,$email){}               

IMHO, this is good approach because I can switch databases by editing just a few lines and my loadParams.php file actually sets parameters programatically based on which host it is:

It is really not the responsibility of this User Model to deal with connection errors or to send a response back to the user about those errors. Pass it in a DB instance that is ready to go. Just think about your architecture and hopefully you can determine where that error reporting should go. If you need to, add a method to this class to report errors to code that uses it. Then it can report error information up the call stack in a organized way.

So, with your suggestion, I should probably make the constructor of my Users class accept $username because all of the public methods will require the username to be set. Then, other methods would accept only specific parameters. Right?

If it has the methods you list, then passing the DB to the constructor seem like it is ready to use to create accounts or login the user.

[jkon]

<Christopher>
"PS - I'd recommend protected over private, unless you absolutely know that you want to forbid a child class from accessing a property.:

I agree with this adding “directly” after the end of the sentence. Remember bicycle for example gear is something that all bicycles have, but should be private in bicycle because it might need to do something more when someone sets the gear (when you create those setters and getters probably no one do anything but you don't know in future). So a mountainBike shouldn't change the gear itself but rather set the gear, if this specific bicycle wants to have a more composite functionality when the gear is set then it should override its parent setGear() and preferably call it and then do its own things.

[Christopher]

"PS - I'd recommend protected over private, unless you absolutely know that you want to forbid a child class from accessing a property.:

I agree with this adding “directly” after the end of the sentence. Remember bicycle for example gear is something that all bicycles have, but should be private in bicycle because it might need to do something more when someone sets the gear (when you create those setters and getters probably no one do anything but you don't know in future). So a mountainBike shouldn't change the gear itself but rather set the gear, if this specific bicycle wants to have a more composite functionality when the gear is set then it should override its parent setGear() and preferably call it and then do its own things.

I think we are generally saying the same thing. Keeping implementation details private is often a good idea, however it is not always the best thing to do. Getters and setters are also not always a good idea because promote an assignment style of access. In your bicycle example, better to have shiftUp() and shiftDown() methods if you want to abstract the gear operation.

Rmember that part of the power of inheritance (over composition) is that it provides direct access to implementation details. You want to protect from mistakes, but protecting yourself from yourself with needless abstraction can produce worse code. You definitely don't want to end up with code like $this->setGear($this->getGear() + 1) ...