PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Sat Jul 22, 2017 3:40 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 8 posts ] 
Author Message
PostPosted: Sun Mar 08, 2015 5:47 pm 
Offline
Forum Newbie

Joined: Sun Mar 08, 2015 5:08 pm
Posts: 3
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:

Syntax: [ Download ] [ Hide ]
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:

Syntax: [ Download ] [ Hide ]
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.


Top
 Profile  
 
PostPosted: Sun Mar 08, 2015 6:30 pm 
Offline
Spammer :|
User avatar

Joined: Wed Oct 15, 2008 2:35 am
Posts: 6457
Location: WA, USA
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:
Syntax: [ Download ] [ Hide ]
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
Syntax: [ Download ] [ Hide ]
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() {
                //
        }

}


Top
 Profile  
 
PostPosted: Mon Mar 09, 2015 3:01 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13417
Location: New York, NY, US
nighthawk wrote:
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.
nighthawk wrote:
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.
nighthawk wrote:
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:
Syntax: [ Download ] [ Hide ]
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.
Syntax: [ Download ] [ Hide ]
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.

_________________
(#10850)


Top
 Profile  
 
PostPosted: Tue Mar 10, 2015 5:54 pm 
Offline
Forum Newbie

Joined: Sun Mar 08, 2015 5:08 pm
Posts: 3
Christopher wrote:
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.

Syntax: [ Download ] [ Hide ]
<?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:

Syntax: [ Download ] [ Hide ]
<?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.


Top
 Profile  
 
PostPosted: Tue Mar 10, 2015 7:29 pm 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6254
Location: Montreal, Canada
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.

_________________
Supported PHP versions No longer supported versions


Top
 Profile  
 
PostPosted: Tue Mar 10, 2015 11:18 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13417
Location: New York, NY, US
nighthawk wrote:
Christopher wrote:
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 !!!!!!!!!
Syntax: [ Download ] [ Hide ]
<?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){}              
 


nighthawk wrote:
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.

nighthawk wrote:
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.

_________________
(#10850)


Top
 Profile  
 
PostPosted: Wed Apr 08, 2015 4:29 am 
Offline
Forum Newbie

Joined: Mon Feb 15, 2010 7:01 am
Posts: 16
<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.


Top
 Profile  
 
PostPosted: Wed Apr 08, 2015 11:18 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13417
Location: New York, NY, US
jkon wrote:
Christopher wrote:
"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) ...

_________________
(#10850)


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 8 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: Exabot [Bot] and 4 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group