Is there anything wrong with taking immediate actions in constructors?

Question

I have classes like this one:

class SomeObject
{
    public function __construct($param1, $param2)
    {
       $this->process($param1, $param2);
    }
    ...
}

So I can instantly "call" it as some sort of global function just like

new SomeObject($arg1, $arg2);

which has the benefits of

• staying concise,
• being easy to understand,

but might break unwritten rules of semantics by not waiting till a method is called.

Should I continue to feel bad because of a bad practice, or there's really nothing to worry about?

Clarification:

• I do want an instance of the class.
• I do use internal methods of the class only.
• I initialize the object in the constructor, but call the "important" action-taker methods too.
• I am selfish in the light of these sentences.

Example:

To give you an idea how I usually use this approach:

new Email('[email protected]', 'Subject line', 'Body Text');

I avoid to overuse it, of course, but in my opinion, this is really handy.

Answer 1

This is a bad practice.

Using a constructor as a global function is not at all what it is intended for, and it would easily be confused. It is not easy to understand at all.

If you want a global function, declare one:

function myGlobalFunction(){ return $something; }

It really isn't that hard...

and even if you aren't using it as a function, you should use the constructor to do just that, construct an object. If you do more than that, you aren't using it for the right purpose, and future contributors will probably get confused quickly.

So, program in a way the makes sense. It really isn't that hard. A few extra keystroke can save you a lot of confusion.

If you need to always take certain actions after making a new instance of a class, try a factory:

 class myFactory{
       public static function makeObject(){
           $obj = new Object();
           $obj->init1();
           return $obj;
       }
 }

 $obj = myFactory::makeObject();

Answer 2

if the code in the constructor is part of creating and initializing the object for use, then I would put it there, but thats me personally, some people may disagree

however, it looks like what you are doing is not intended for building the object/class but doing some other process. this is bad, and should be done in a separate method.

Keep the constructor for construction.

Answer 3

Occam's Razor says entia non sunt multiplicanda praeter necessitatem, literally: "entities must not be multiplied beyond necessity". There's nothing wrong with doing work in a constructor[*], but don't require callers to create an object they don't use, just to get the side-effects of its constructor.

[*] Assuming that you're comfortable with the mechanism that your programming language uses to indicate the failure of a constructor. Returning a success/failure code out of a constructor might not be an option, so if you want to avoid an exception in the failure case, then you might be reduced to setting "is usable" flags in the object, which isn't great for usability either.

Answer 4

To my mind, it depends on what the purpose of the function is.

For example, something that I feel would be entirely appropriate to call at the end of a constructor might be something like a refreshCache() function. It's a real public function as other objects may wish to call it, but at the same time you know it's the same logic that you want to apply initially while setting up your object.

In a general sense, I suppose the kind of (public) functions you'd want to call during a constructor are typically idempotent functions that manipulate the local state, so often things like cache population/prefetching things/etc.

If you have an object that does some kind of processing on an input, I would definitely avoid

s = new MyObject(a, b)
return s.getResult()

in favour of

s = new MyObject() // perhaps this is done elsewhere, even once at startup
return s.process(a, b)

as arguably the latter is clearer as to what is actually happening (especially if you give process() a real name :-)), and lets you reuse a single, lighter-weight object for the calculations, in turn making it easier to pass around.

Of course, if you have quite a complex constructor (this is probably a bad thing in general, but doubtless there are some cases where it makes sense), then it's never a bad thing to separate a block of related functionality out into a private function for readability.

Answer 5

This is concise, but it is not remotely idiomatic, so it's not easy for others to understand.

In the future, it may not even be easy for you to understand it.

It may not be important to you, but one problem with this is it makes your classes harder to test - see Flaw: Constructor does Real Work.

Answer 6

I think this is bad practice for two reasons.

• It is not clear to the caller that other operations above and beyond what is minimally required for initialization are taking place.
• It leads to awkward coding scenarios if this operation throws exception.

Regarding the first point...there is an implicit assumption that constructors only perform enough work to construct an object in a defined and consistent state. Placing extra work in them can lead to confusion if the work is long running or IO bound. Remember, the constructor cannot convey any meaning through its name like methods. One alternative is to create a static factory method with a meaningful name that returns a new instance.

Regarding the second point...if the constructor contains an operation that throws exceptions unpredictably (contrasted with exceptions thrown because of parameter validation for example) then your exception handling code gets awkward. Consider the following example in C#. Notice how the good design has a more elegant feel to it. Nevermind, the fact that the clearly named method is at least an order of magnitude more readable.

public class BadDesign : IDisposable
{
  public BadDesign()
  {
    PerformIOBoundOperation();
  }

  private void PerformIOBoundOperation() { }
}

public class GoodDesign : IDisposable
{
  public GoodDesign()
  {

  }

  public void PerformIOBoundOperation() { }
}

public static void Main()
{
  BadDesign bad = null;
  try
  {
    bad = new BadDesign();
  }
  catch
  {
    // 'bad' is left as null reference. There is nothing more we can do.
  }
  finally
  {
    if (bad != null)
    {
      bad.Dispose();
    }
  }

  GoodDesign good = new GoodDesign();
  try
  {
    good.PerformIOBoundOperation();
  }
  catch
  {
    // Do something to 'good' to recover from the error.
  }
  finally
  {
    good.Dispose();
  }
}

Answer 7

Things to be concerned with regarding this pattern:

• Breaking Single Responsibility Principle
• Implementing Functional Decomposition Antipattern
• Failing to meet common consumer expectations

Single Responsibility Principle

This is a slight variation on the original principle, but applied instead to functions. If your constructor is doing more than one thing (constructing and processing) you make the maintenance of this method more difficult in the future. If a caller doesn't want to process during construction, you leave him no option. If the "processing" one day requires additional steps that shouldn't be in the constructor, you have to refactor everywhere you use this method.

Functional Decomposition Antipattern

Without knowing specifics, I'd be concerned that code that does this implements this antipattern. The objects aren't truly objects, but functional programming units wrapped in object oriented disguise.

Common consumer expectations

Would the average caller expect this behavior of the constructor? There might be places where extra processing during construction can be a short hand programming convenience. But this should be explicitly documented and well understood by the callers. The overall use of the object should still make sense in this context and it should be natural for the caller to use it in this fashion.

Also understand what you are forcing on your consumer. If you are doing processing in the constructor, you are forcing the consumer to pay for that processing (in terms of processing time) whether they want it or not. You eliminate the possibility of doing "lazy" processing or other forms of optimization.

Acceptable usage?

Only in places where common use of the class requires initialization that can be optional at construction time. The File class is a good example:

/* This is a common usage of this class */
File f;
f.Open(path);

/* This constructor constructs the object and puts it in the open state in one step 
 * for convenience */
File f(path);

But even this is questionable. Does the file class keep the path to the file internally or is it just used to open a file handle? If it does store the file path, now Open() and the new constructor have more than one responsibility ( SetPath(p) and Open() ). In that case, maybe the File(path) convenience constructor shouldn't open the file, but should rather just set the path in the object.

There are lots of considerations based on the objects in question. Consider writing unit tests for your objects. They will help you work out a lot of these use case issues.

Answer 8

Your class is technically fine, and it might simply come down to personal philosophy.

but to others reading this that might get some crazy ideas about constructors, read this:

http://www.ibm.com/developerworks/java/library/j-jtp0618.html

If you're going to start throwing around the "this" reference, you can end up in a lot of trouble, especially in multi-threaded environments. Just an FYI.