SitePoint Sponsor

User Tag List

Results 1 to 21 of 21
  1. #1
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)

    Domain Model setter methods return $this, good or bad practice?

    I remember reading an online article stating that void methods are poor programming practice and that every method needs to return something. He makes a good point, although apparently methods with return-values can cause more benchmark issues. One possible usage I can see from converting void methods into standard methods is when it comes down to domain model's setter methods. Consider the User class below, with setter methods returning $this the client code may look more elegant to some people since it enables method chaining:

    PHP Code:
    public class User extends DomainModel{
        protected 
    $id// getter/setter for id property are defined in DomainModel class...
        
    protected $username;
        protected 
    $password;
        protected 
    $email;

        public function 
    getUsername(){
            return 
    $this->username;
        }
        
        public function 
    setUsername($username){
            
    $this->username $username;
            return 
    $this;
        }

        public function 
    getPassword(){
            return 
    $this->password;
        }
        
        public function 
    setPassword($password){
            
    $this->password $password;
            return 
    $this;
        }

        public function 
    getEmail(){
            return 
    $this->email;
        }
        
        public function 
    setEmail($email){
            
    $this->email $email;
            return 
    $this;
        }
    }

    $user = new User(1);
    $user->setUsername("Admin")->setPassword("000000")->setEmail("admin@yoursite.com"); 
    But I do not see this being applied widely in any PHP frameworks, so I assume there are some severe disadvantages(perhaps more than just benchmark concerns?) to having setter methods returning the object itself($this). Is it a poor OOP practice to have Domain Model's setter methods returning $this? If so, what are the problems? Id like to learn about them, and thx.

  2. #2
    SitePoint Wizard silver trophybronze trophy Stormrider's Avatar
    Join Date
    Sep 2006
    Location
    Nottingham, UK
    Posts
    3,133
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    What are the reasons for saying "methods that don't return anything are bad practice"? What's the justification behind that? Would be good to understand that, might make the question easier to answer.

  3. #3
    Non-Member
    Join Date
    Oct 2007
    Posts
    363
    Mentioned
    11 Post(s)
    Tagged
    0 Thread(s)
    I do this in my model entities all the time. I've never experienced issues with it. As you say, it allows for method chaining.

    I'd be interested to hear whether people can point out some down sides to doing this?

  4. #4
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    Well it was from Brandon Savage's Mastering Object Oriented PHP e-book that I purchased a few months ago. In the second section of chapter 11: Good Object Oriented Citizenship, his rule #2 was to always return something. Here is what he said:

    The practice of not returning a value is dangerous, and it prevents certain behaviors that are to some degree considered beneficial.
    The reason that you often return a value is to evaluate what that value was. For example, if the setter succeeded, returning TRUE would allow the developer to evaluate that behavior. A FALSE response would indicate failure (or an exception).
    Another option in more simple functions like the one illustrated above is to return the object itself by simply returning $this. By returning $this, you can chain setter calls together.
    I cant quote the entire section since it contains lots of source code, also I am afraid that it may be copyright infringement if I include the e-book's source code here.

    Quote Originally Posted by aaarrrggh View Post
    I do this in my model entities all the time. I've never experienced issues with it. As you say, it allows for method chaining.

    I'd be interested to hear whether people can point out some down sides to doing this?
    Maybe benchmark issue? I thought void methods are 1.2-1.5x faster than value-returning methods?

  5. #5
    Non-Member
    Join Date
    Oct 2007
    Posts
    363
    Mentioned
    11 Post(s)
    Tagged
    0 Thread(s)
    Not sure I agree with the first argument here with regards to always having to return a value. If you have a decent suite of unit tests, most of the getters and setters would be tested anyway.

    I do return $this on setters in a lot of my models, just because I find method chaining to be very convenient and fluid.

    In terms of the benchmark - I'm not sure on that one. I've never ran benchmarks to test this, so I couldn't say.

    A google search took me here: http://programmers.stackexchange.com...-encapsulation

    I guess the argument could be made that relying upon objects that return $this breaks encapsulation because you're relying on that always being the case, but then you're always tied to the interface of the objects you use in one form or another anyway... Not sure I quite get the reason this breaks encapsulation?

    Quote Originally Posted by Hall of Famer View Post
    Well it was from Brandon Savage's Mastering Object Oriented PHP e-book that I purchased a few months ago. In the second section of chapter 11: Good Object Oriented Citizenship, his rule #2 was to always return something. Here is what he said:



    I cant quote the entire section since it contains lots of source code, also I am afraid that it may be copyright infringement if I include the e-book's source code here.



    Maybe benchmark issue? I thought void methods are 1.2-1.5x faster than value-returning methods?

  6. #6
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    umm from the link you provided, it seems to me that method chaining is considered a poor OOP practice... Never knew this before, but yeah it does seem to me that method chaining breaks encapsulation. I'd say perhaps method chaining may and only may be used in situations in which you are at least 99% sure that you will always be calling a series of methods together. One example is a query object, in which you always have select(), where(), limit() and order() methods chaining together to form a complete query operation. There are definitely a lot more counter examples though from the way it looks.

  7. #7
    Non-Member
    Join Date
    Oct 2007
    Posts
    363
    Mentioned
    11 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Hall of Famer View Post
    umm from the link you provided, it seems to me that method chaining is considered a poor OOP practice... Never knew this before, but yeah it does seem to me that method chaining breaks encapsulation. I'd say perhaps method chaining may and only may be used in situations in which you are at least 99% sure that you will always be calling a series of methods together. One example is a query object, in which you always have select(), where(), limit() and order() methods chaining together to form a complete query operation. There are definitely a lot more counter examples though from the way it looks.
    Yeah, it's interesting to think about. I'm currently in the early days on a project, so I may consider changing the way these models work. It wouldn't be hard to change at this stage.

    I do see what you mean about the query objects - it makes perfect sense for something like that. It'd be the same for a builder object I guess. In terms of standard models, perhaps it's not the best idea...

  8. #8
    Non-Member
    Join Date
    Oct 2007
    Posts
    363
    Mentioned
    11 Post(s)
    Tagged
    0 Thread(s)
    The advantage of following the Law of Demeter is that the resulting software tends to be more maintainable and adaptable. Since objects are less dependent on the internal structure of other objects, object containers can be changed without reworking their callers.
    Hmm... Yeah, think I may stop using this technique for general entities...

  9. #9
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    Well if you are using Doctrine-like Entities, Id say it may not be a bad idea for method chaining. As far as I know, Doctrine Entities only have getters and setters, Id say its actually sorta like anemic domain model but whatever... In many cases though, your domain model will have more business logic than simple getters and setters, in these scenarios method chaining can be a serious threat to encapsulation. So yeah, its really difficult to reach a general agreement.

  10. #10
    Non-Member
    Join Date
    Oct 2007
    Posts
    363
    Mentioned
    11 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Hall of Famer View Post
    Well if you are using Doctrine-like Entities, Id say it may not be a bad idea for method chaining. As far as I know, Doctrine Entities only have getters and setters, Id say its actually sorta like anemic domain model but whatever... In many cases though, your domain model will have more business logic than simple getters and setters, in these scenarios method chaining can be a serious threat to encapsulation. So yeah, its really difficult to reach a general agreement.
    Yeah. I found the anemic model stuff interesting, but I'm still using that technique right now because I've had a lot of success with it.

    Interestingly, if you use the Symfony 2 framework and get it to auto generate entities for you from the command line, it returns $this in setters by default.

  11. #11
    Community Advisor bronze trophy
    fretburner's Avatar
    Join Date
    Apr 2013
    Location
    Brazil
    Posts
    1,400
    Mentioned
    45 Post(s)
    Tagged
    12 Thread(s)
    I came across a couple of posts on Benjamin Eberlei's blog, which suggest that it's better OO practice to try and avoid getters/setters in favour of use-case driven methods instead:
    http://www.whitewashing.de/2012/08/2...s_allowed.html
    http://www.whitewashing.de/2013/02/0...and_solid.html

    I'd be interested to know what people's options are on this, and if anyone has tried this?

  12. #12
    SitePoint Wizard silver trophybronze trophy Stormrider's Avatar
    Join Date
    Sep 2006
    Location
    Nottingham, UK
    Posts
    3,133
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Hall of Famer View Post
    umm from the link you provided, it seems to me that method chaining is considered a poor OOP practice... Never knew this before, but yeah it does seem to me that method chaining breaks encapsulation. I'd say perhaps method chaining may and only may be used in situations in which you are at least 99% sure that you will always be calling a series of methods together. One example is a query object, in which you always have select(), where(), limit() and order() methods chaining together to form a complete query operation. There are definitely a lot more counter examples though from the way it looks.
    The law of Demeter doesn't apply here since you are returning the same object, not a different one - method chaining doesn't break encapsulation, unless you are chaining methods from many different classes, though that's not usually what the term is used to mean.

  13. #13
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,264
    Mentioned
    18 Post(s)
    Tagged
    0 Thread(s)
    Seems like I agree with most everyone else here.

    I've never known void methods to be considered poor practice, and certainly not dangerous. Nor do I see how returning $this breaks encapsulation. You're returning the same object that the caller already has access to. I also can't imagine it would affect performance in any significant way, though certainly feel free to measure and double check.

    If we do choose to return $this, it's only for the convenience of chaining. I can't say how widely this is done in PHP frameworks, but I can say that Symfony does it.
    "First make it work. Then make it better."

  14. #14
    SitePoint Guru
    Join Date
    Nov 2003
    Location
    Huntsville AL
    Posts
    689
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    For example, if the setter succeeded, returning TRUE would allow the developer to evaluate that behavior. A FALSE response would indicate failure (or an exception).
    How old is Savage's book? I searched for it but I didn't find when it was published.

    If it was originally written before PHP exceptions were common then he has a good point. Checking that functions/methods have worked is a good thing. How often do you still see: "$db = openConnection(); if (!$db) then whoops" style code.

    But thanks to exceptions(which he mentions) we can be certain that the method worked as expected without individual checks. And since a setters only job should be to set a value then it makes no sense for it to return anything specific. If something goes wrong then an exception is thrown.

    I myself don't use fluent interfaces but that is just me. Returning $this is a bit of a hack perhaps but it seems to work fine in practice.

  15. #15
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    Umm so the discussion takes a turn now? I am a bit confused, from what aaarrrgh showed me last night it was clearly that Method chaining was close to an anti-pattern, but all of a sudden its getting complicated.

  16. #16
    SitePoint Guru
    Join Date
    Nov 2003
    Location
    Huntsville AL
    Posts
    689
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by fretburner View Post
    I came across a couple of posts on Benjamin Eberlei's blog, which suggest that it's better OO practice to try and avoid getters/setters in favour of use-case driven methods instead:
    http://www.whitewashing.de/2012/08/2...s_allowed.html
    http://www.whitewashing.de/2013/02/0...and_solid.html

    I'd be interested to know what people's options are on this, and if anyone has tried this?
    Here is another of his articles in which he essentially adds a domain layer to doctrine: http://www.whitewashing.de/2013/07/2...ainevents.html

    This is also another DDD/Symfony article in progress: http://williamdurand.fr/2013/08/07/d...nd-code-first/

    I have been watching with interest the notion of implementing some of the Domain Driven Design(DDD) patterns with PHP. Sadly, my own attempts so far have failed. The amount of infrastructure required (as opposed to crud and anemic data models) makes the benefits of my implementations unclear. But if Mr Eberlei's is working on it then there is hope.

  17. #17
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    Quote Originally Posted by ahundiak View Post
    How old is Savage's book? I searched for it but I didn't find when it was published.

    If it was originally written before PHP exceptions were common then he has a good point. Checking that functions/methods have worked is a good thing. How often do you still see: "$db = openConnection(); if (!$db) then whoops" style code.

    But thanks to exceptions(which he mentions) we can be certain that the method worked as expected without individual checks. And since a setters only job should be to set a value then it makes no sense for it to return anything specific. If something goes wrong then an exception is thrown.

    I myself don't use fluent interfaces but that is just me. Returning $this is a bit of a hack perhaps but it seems to work fine in practice.
    Savage's book should be pretty new, as he talks about traits and closure.

  18. #18
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,264
    Mentioned
    18 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Hall of Famer View Post
    Umm so the discussion takes a turn now? I am a bit confused, from what aaarrrgh showed me last night it was clearly that Method chaining was close to an anti-pattern, but all of a sudden its getting complicated.
    I looked at that link to the StackExchange question. The poster there isn't describing method chaining as I know it. He compares:

    main.getA().getB().getC().transmogrify(x, y)

    with

    main.getA().transmogrifyMyC(x, y)

    But actually a non-chaining example would look more like this:

    main.getA()
    main.getB()
    main.getC()
    main.transmogrify(x, y)


    The poster on SE is talking about something different and much more funky.
    "First make it work. Then make it better."

  19. #19
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,264
    Mentioned
    18 Post(s)
    Tagged
    0 Thread(s)
    I suppose the kind of method chaining I'm thinking of is also called a fluent interface.
    "First make it work. Then make it better."

  20. #20
    SitePoint Addict bronze trophy
    Join Date
    Apr 2013
    Location
    Ithaca
    Posts
    351
    Mentioned
    6 Post(s)
    Tagged
    1 Thread(s)
    I see, so even Method chaining can be broken down into different subparts, with one of them being fluent interface? Thanks for the information, it helps a lot. I was wondering though, why isnt smalltalk's cascade feature imported into other object oriented programming languages? From the way it sounds, smalltalk's cascade feature inspires method chaining, but its more powerful than method chaining.

  21. #21
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,264
    Mentioned
    18 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Hall of Famer View Post
    I was wondering though, why isnt smalltalk's cascade feature imported into other object oriented programming languages? From the way it sounds, smalltalk's cascade feature inspires method chaining, but its more powerful than method chaining.
    My best guess is because method chaining is easy to implement, so there aren't many people clamoring to have the language do it for us.
    "First make it work. Then make it better."


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •