Separation of Concerns – Should All Work Be Done in a Constructor?

constructorslanguage-agnosticseparation-of-concerns

Let me preface this by saying this is not my code nor my coworkers' code. Years ago when our company was smaller, we had some projects we needed done that we did not have the capacity for, so they were outsourced. Now, I have nothing against outsourcing or contractors in general, but the codebase they produced is a mass of WTFs. That being said, it does (mostly) work, so I suppose it's in the top 10% of outsourced projects I've seen.

As our company has grown, we've tried to take more of our development in house. This particular project landed in my lap so I've been going over it, cleaning it up, adding tests, etc etc.

There's one pattern I see repeated a lot and it seems so mindblowingly awful that I wondered if maybe there is a reason and I just don't see it. The pattern is an object with no public methods or members, just a public constructor that does all the work of the object.

For example, (the code is in Java, if that matters, but I hope this to be a more general question):

public class Foo {
  private int bar;
  private String baz;

  public Foo(File f) {
    execute(f);
  }

  private void execute(File f) {
     // FTP the file to some hardcoded location, 
     // or parse the file and commit to the database, or whatever
  }
}

If you're wondering, this type of code is often called in the following manner:

for(File f : someListOfFiles) {
   new Foo(f);
}

Now, I was taught long ago that instantiated objects in a loop is generally a bad idea, and that constructors should do a minimum of work. Looking at this code it looks like it would be better to drop the constructor and make execute a public static method.

I did ask the contractor why it was done this way, and the response I got was "We can change it if you want". Which was not really helpful.

Anyway, is there ever a reason to do something like this, in any programming language, or is this just another submission to the Daily WTF?

Best Answer

Ok, going down the list:

I was taught long ago that instantiated objects in a loop is generally a bad idea

Not in any languages I've used.

In C it is a good idea to declare your variables up front, but that is different from what you said. It might be slightly faster if you declare objects above the loop and reuse them, but there are plenty of languages where this increase in speed will be meaningless (and probably some compilers out there that do the optimization for you :) ).

In general, if you need an object within a loop, create one.

constructors should do a minimum of work

Constructors should instantiate the fields of an object and do any other initialization necessary to make the object ready to use. This is generally means constructors are small, but there are scenarios where this would be a substantial amount of work.

is there ever a reason to do something like this, in any programming language, or is this just another submission to the Daily WTF?

It's a submission to the Daily WTF. Granted, there are worse things you can do with code. The problem is that the author has a vast misunderstanding of what classes are and how to use them. Specifically, here's what I see that is wrong with this code:

  • Misuse of classes: The class is basically acting like a function. As you mentioned, it should either be replaced with a static function, or the function should just be implemented in the class that is calling it. Depends on what it does and where it is used.
  • Performance overhead: Depending on the language, creating an object can be slower than calling a function.
  • General confusion: It is generally confusing to the programmer how to use this code. Without seeing it used, no one would know how the author intended to use the code in question.
Related Topic