Encapsulation - our way



  •   public class X {
    // OP: comment below quoted verbatim
    // The call to construct this class must ALWAYS be paired with the call
    // to: setUpInternalStuff(), or the class won't be initialized correctly
    public X() { ... }

    public void setUpInternalStuff() { ...}
    

    }


  • sockdevs

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?



  • Just that the original author should have deleted the comments, and everything between and including the closing brace of the constructor and the opening brace of the method.


  • sockdevs

    But what if the method is setting up stuff that can't be done in the constructor? Like I said, without knowing the contents, is it really a WTF?

    Edit: Maybe there's an asynchronous operation that needs to finish before the class can be used. It's generally bad practice to call async code from a constructor, at least based on a quick Googling. Therefore, the async operation has to be in a separate initialisation method.



  • @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?

    Sadly, yes. setUpInternalStuff included all the control variables for the class.

    [Edit] I just saw your subsequent post. No async stuff was involved.



  • Any virtual methods? (see [url]http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor[/url] )

    we have similar cases in C#, where the initializer calls virtual methods (so it's not a good idea to put it in the constructor)



  • @snoofle said:

    @RaceProUK said:

     Without knowing what's in <FONT face="courier new,courier">setUpInternalStuff()</FONT>, is it really a WTF?

    Sadly, yes. setUpInternalStuff included all the control variables for the class.

    [Edit] I just saw your subsequent post. No async stuff was involved.

    RaceProUK's remark is really important. If the 'setUp' function is creating a hierachy of child objects, all this functionality should be taken out and put in an XFactory class which would pass in everything via the constructor: dependency injection to make X a lot more testable

  • sockdevs



  • @racemaniac said:

    Any virtual methods? (see http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor )

    we have similar cases in C#, where the initializer calls virtual methods (so it's not a good idea to put it in the constructor)
    No virtual methods. No nothing. Just 50 lines of assignments and internal calculations.



  • Guys you can trust if Snoofle posted it, it's a real WTF. Ok? Geez.

    "Snoofle, I know the last 347234623843724,4324873285623857623,4329423843282423 WTFs you posted were real, BUT WHAT IF THERE IS A ASYNC CODE IN THE CONSTRUCTOR the gods must be crazy!!!"


  • sockdevs



  • @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?

    Not a WTF as what snoofle has us accustomed, but in this case you should make the constructor private and provide a static instanceOf method which does all the required steps, not an stupid comment which you'll only read when an instance of this class shits on itself without a reason. Also, putting ANY CODE IN THE CONSTRUCTOR IS A WTF!... except of course of variable initialization.

    And I don't see how a Factory would work here.



  • @ubersoldat said:

    @RaceProUK said:

     Without knowing what's in <FONT face="courier new,courier">setUpInternalStuff()</FONT>, is it really a WTF?

    Not a WTF as what snoofle has us accustomed, but in this case you should make the constructor private and provide a static instanceOf method which does all the required steps, not an stupid comment which you'll only read when an instance of this class shits on itself without a reason. Also, putting ANY CODE IN THE CONSTRUCTOR IS A WTF!... except of course of variable initialization.

    And I don't see how a Factory would work here.

    I agree with your Factory remark because we don't know what is in "setUpInternalStuff()"

    Example of where a factory would be useful to outsource the tedious work of building the object graph: (Courtesy of http://googletesting.blogspot.be/2008/08/by-miko-hevery-so-you-decided-to.html#!/2008/07/how-to-think-about-new-operator-with.html)

    class House {
      private Kitchen kitchen;
    

    public House(Kitchen kitchen ) {
    this.kitchen = kitchen
    }

    class HomeBuilder {

    House build() {

    return new House(
             new Kitchen(new Sink(), new Dishwasher(), new Refrigerator())
           );
    

    }
    }

    instead of

    class House {
      private Kitchen kitchen;
    

    public House( ) {
    // something
    }

    public setUpInternalStuff() {
    this.kitchen = new Kitchen(new Sink(), new Dishwasher(), new Refrigerator());
    }
    }



  • @racemaniac said:

    Any virtual methods? (see http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor )

    we have similar cases in C#, where the initializer calls virtual methods (so it's not a good idea to put it in the constructor)

    Since this is java...Everything* is virtual!

    * OBPedanticDickweed: Everything that isn't static. Or private. Maybe some final methods.



  • @racemaniac said:

    Any virtual methods? (see http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor )

    we have similar cases in C#, where the initializer calls virtual methods (so it's not a good idea to put it in the constructor)

    For C#.... 

    Virtual calls in SEALED classes are perfectly acceptable as the behaviour is 100% deterministic. Since all non-leaf classes should be abstract, and all leaf classes should be sealed, all is good.



  • @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?
     

    agreed, if setUpInternalStuff uses global variables that need to be set between the constructor and actual call to this function, then it's perfectly reasonable.



  • @SEMI-HYBRID code said:

    @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?
     

    agreed, if setUpInternalStuff uses global variables that need to be set between the constructor and actual call to this function, then it's perfectly reasonable.

    Sorry, it doesn't;

    I know there are times when this sort of arrangement might make sense, but in this case, there is absolutely NO justification for it; they just followed their standard decapsulation anti pattern.



  • I assume you've checked that setUpInternalStuff isn't reused outside the constructor.

    Out of curiosity, does the class implement Serializable? If so, does readObject call setUpInternalStuff?

    Edit: never mind. There's no "implements Serializable". Missed an opportunity for a WTF there.



  • @SEMI-HYBRID code said:

    @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?
     

    agreed, if setUpInternalStuff uses global variables that need to be set between the constructor and actual call to this function, then it's perfectly reasonable.

    Bonus points if it queries an external web service that is provided by the same application.



  • @RaceProUK said:

     

     

    There's no way I can swallow a pill that large.



  • @ubersoldat said:

    Also, putting ANY CODE IN THE CONSTRUCTOR IS A WTF!... except of course of variable initialization.
    Why, pray tell?

     



  • setUpInternalStuff() is public. Calling non private methods in a java constructor is not recommended. Somebody probably told him that, with such awful result.

    Note: i have my fair amount of "initialize()" methods in a project, but at least it's because data injection is needed before doing any initialization stuff. And aop takes care of calling the initialize, not the future programmers :s


  • sockdevs

    @TheCPUWizard said:

    Since all non-leaf classes should be abstract, and all leaf classes should be sealed, all is good.

    Unfortunately, that sounds like the sort of holier-than-thou garbage that the so-called 'OO purists' put out to sound superior. Then they come unstuck when they meet real world requirements that are best met by an instantiable class that is also subclassable (is that a word? It is now).



  • @snoofle said:

    @SEMI-HYBRID code said:

    @RaceProUK said:

     Without knowing what's in <font face="courier new,courier">setUpInternalStuff()</font>, is it really a WTF?
     

    agreed, if setUpInternalStuff uses global variables that need to be set between the constructor and actual call to this function, then it's perfectly reasonable.

    Sorry, it doesn't;

    I know there are times when this sort of arrangement might make sense, but in this case, there is absolutely NO justification for it; they just followed their standard decapsulation anti pattern.


    I primarily meant it as a jokey sarcastic remark, not an actual guess at why it's done that way, but good to know, I guess.



  • @RaceProUK said:

    @TheCPUWizard said:

    Since all non-leaf classes should be abstract, and all leaf classes should be sealed, all is good.

    Unfortunately, that sounds like the sort of holier-than-thou garbage that the so-called 'OO purists' put out to sound superior. Then they come unstuck when they meet real world requirements that are best met by an instantiable class that is also subclassable (is that a word? It is now).

    Actually it has been quite practical across the projects I have worked on for the past decade (remembering that my comment was directed at a reply regarding C#, I do not do significant enough Java to have an informed, real-world opinion there). Inheritable (subclassable) classes are much harded to design, sealing a concrete class eliminates a large number of potential problems. I have not found a need for a class which is both concrete and inheritable yet. If it appears that such a condition exists, then a "general" subclass (sealed, of course) meets the bill quite nicely, and is 100% explicit.



  • There are good reasons at times to use two-stage construction

    1. The second stage (init or whatever) can be virtual or call virtual functions or register ourselves with other objects

    Interesting that dependency injection has been mentioned here as my dependency injection system invokes a call to a virtual "bindParams" method after the constructor call, which incidentally takes as a parameter a reference to (the base class of) the object that invokes it.

    2. Works well when you have many constructors and one init() method.

     

     


  • sockdevs

    @TheCPUWizard said:

    @RaceProUK said:

    @TheCPUWizard said:

    Since all non-leaf classes should be abstract, and all leaf classes should be sealed, all is good.

    Unfortunately, that sounds like the sort of holier-than-thou garbage that the so-called 'OO purists' put out to sound superior. Then they come unstuck when they meet real world requirements that are best met by an instantiable class that is also subclassable (is that a word? It is now).

    Actually it has been quite practical across the projects I have worked on for the past decade (remembering that my comment was directed at a reply regarding C#, I do not do significant enough Java to have an informed, real-world opinion there). Inheritable (subclassable) classes are much harded to design, sealing a concrete class eliminates a large number of potential problems. I have not found a need for a class which is both concrete and inheritable yet. If it appears that such a condition exists, then a "general" subclass (sealed, of course) meets the bill quite nicely, and is 100% explicit.

    No argument from me - if it works, then it works :) I just do it differently.



  • @TheCPUWizard said:

    If it appears that such a condition exists, then a "general" subclass (sealed, of course) meets the bill quite nicely, and is 100% explicit.
     

     

    Actually, I've seen somebody try to do this a few times, to follow "good principle"...

     

    What they ended up with was, in order to make the class general, they accepted functions in the constructor for specific actions. That is, instances of a delegate type. These were also exposed as properties.

     Now, I have nothing against that fundamentally- accepting functions as arguments can be very useful for generalizing code.But what they did here was essentially factor out the virtual methods into delegate properties, and in each base implementation check if the corresponding property was null and if not call it, otherwise, do the "normal" thing. Then "creating" one of the derived types was as simple as using a new  factory class that was designed for creating instances of the class with the appropriate function properties set to reflect the derived class implementation.

    My first response was "Jesus" My second response was "Christ"...

    Basically, There is such a thing as going too extreme. I've seen the same thing in the other direction, too- subclasses and implemented interfaces all over the place inexplicably.

     

     

     



  • Making all leaf classes final is one of the recommendations of How to Write Unmaintainable Code: After all, your code is perfect from the start and will never need to be extended, ever.



  • @BC_Programmer said:

    @TheCPUWizard said:

    If it appears that such a condition exists, then a "general" subclass (sealed, of course) meets the bill quite nicely, and is 100% explicit.
     

    Actually, I've seen somebody try to do this a few times, to follow "good principle"...

    What they ended up with was, in order to make the class general, they accepted functions in the constructor for specific actions. That is, instances of a delegate type. These were also exposed as properties.

     Now, I have nothing against that fundamentally- accepting functions as arguments can be very useful for generalizing code.But what they did here was essentially factor out the virtual methods into delegate properties, and in each base implementation check if the corresponding property was null and if not call it, otherwise, do the "normal" thing. Then "creating" one of the derived types was as simple as using a new  factory class that was designed for creating instances of the class with the appropriate function properties set to reflect the derived class implementation.

    My first response was "Jesus" My second response was "Christ"...

    Basically, There is such a thing as going too extreme. I've seen the same thing in the other direction, too- subclasses and implemented interfaces all over the place inexplicably.

    the is not what I am saying at all. As a trivial example consider the following:

     class Dog {}
     class Collie : Dog {}
     class Boxer : Dog {}
      // Other breed specific classes....

     Now some people will allow for the instanciation of "Dog". I disagree, you can not have a dog that is not of some type of (possibly mixed, or unknown) breed. I would make Dog abstract, then specifically derive an "UnSpecifiedBreed" class. Functionally there would be ZERO difference between what could be done by allowing instanciation of "Dog" vs. requiring instanciation of "UnSpecifiedBreed"; in some cases, the leaf class would contain NO members, and simply have a sealed attribute rather than the base class's abstract attribute.



  • @Medinoc said:

    Making all leaf classes final is one of the recommendations of How to Write Unmaintainable Code: After all, your code is perfect from the start and will never need to be extended, ever.

    While that site has some merit, it has far more flaws. This has nothing to do with perfection, or the need to extend. It is simply a differentiation between "this class is meant to be extended for specific purposes" and "this class is designed for a specific purpose".


  • sockdevs

    @TheCPUWizard said:

    in some cases, the leaf class would contain NO members, and simply have a sealed attribute rather than the base class's abstract attribute.

    This was the basis of my original objection; I just couldn't think of a good example at the time. If there's no functional difference between a class and its subclass, then you have one class too many.

    I'm not saying the 'only abstract or sealed' doesn't have merit, and agree that in some cases it can work well. What I am saying is it's not a universal solution, and sometimes (more often than not in my experience), having a class both concrete and subclassable is more useful and/or appropriate.

    The way I see it, 'sealed' is a strict limitation that should only be applied when necessary e.g. System.String, and 'abstract' is when you want to define an object where the high-level operations are common to all subclasses, but the low-level is specialised per subclass e.g. System.Data.DBConnection.

    As for the inbetween, let's say I have a TextBox class. I use this in hundreds of places, and it works well. Then, in one place I need a specialised TextBox, say NumberTextBox. Now I have two options:

    1. Subclass TextBox as NumberTextBox
    2. Make TextBox abstract, and create StandardTextBox and NumberTextBox.

    With #1, I have one new class. Risk of messing up existing TextBoxes - none.

    With #2, I have two new classes, and hundreds of (now abstract) TextBoxes to change. Risk of messing up existing TextBoxes - significant.

    Most developers would choose option #1, the simplest and easiest solution.

    Of course, if TextBox was sealed and in a third-party library I can't change, then I'd have a lot of duplication to do. Not to mention option #2 above would be impossible.



  • @TheCPUWizard said:

    UnSpecifiedBreed
    This is TRWTF. Amirite?



  • @Sutherlands said:

    @TheCPUWizard said:
    UnSpecifiedBreed
    This is TRWTF. Amirite?

    UnspecifiedBreed is a completely unrelated class. You can understand the need to avoid a conflict, I'm sure.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.