Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
12  
Looks to me like it was written by developers who know about public static void main(string[] args) and who've heard of objects, then tried to mash them together. –  AndyBursh Aug 2 '12 at 13:40
    
If you are never called again, you must do it there. Many modern frameworks for e.g. dependency injection needs the beans to be made, set properties and THEN invoked, and then you cannot use these heavy workers. –  user1249 Aug 2 '12 at 14:57
3  
Related question: Any problem with doing the main work of a class in its constructor?. This one's a bit different, though, because the created instance is used afterwards. –  sleske Aug 23 '12 at 15:08
    
a good read about Separating object Use from its Construction –  Songo Sep 9 '12 at 10:00
    
I don't really want to expand this to a full answer, but let me just say: there are scenarios, where this is arguably acceptable. This one is very far from it. Here an object is instantiated without actually needing the object. In contrast to that, if you were creating some kind of data-structure from an XML file, initiating the parsing from the constructor is not entirely awful. In fact in languages that allow overloading constructors, it's nothing I would kill you for as a maintainer ;) –  back2dos Oct 7 '12 at 21:12
add comment

10 Answers

up vote 45 down vote accepted

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.
share|improve this answer
    
Thanks for your response. In reference to "instantiating objects in a loop", this is something a lot of static analysis tools warn about, and I also heard that from some college professors. Granted, that might be a hold-over from the 90s when the performance hit was larger. Of course, if you need it, do it, but I'm surprised to hear an opposing viewpoint. Thanks for broadening my horizons. –  Kane Aug 2 '12 at 13:56
7  
Instantiating objects in a loop is a minor code smell; you should look at it and ask "do I really need to instantiate this object multiple times?". If you are instantiating the same object with the same data and telling it to do the same thing, you will save cycles (and avoid thrashing memory) by instantiating it once and then only repeating the instruction with whatever incremental changes may be necessary to object state. However, if you are, for instance, reading a data stream from a DB or other input and creating a series of objects to hold that data, by all means instantiate away. –  KeithS Aug 2 '12 at 14:55
2  
Short version: Don't use object instantiations like they were a function. –  Erik Reppen Aug 2 '12 at 20:24
add comment

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

Yes, may be you are recalling why we need the StringBuilder.

There are two schools of Java.

The builder was promoted by the first one, which teaches us to reuse (because sharing = identity to efficiency). However, in the beginning of 2000 I started to read that creating objects in Java is so cheap (as opposed to C++) and GC is so efficient that reuse is worthless and the only thing that matters is productivity of programmer. For productivity, he must write manageable code, which means modularization (aka narrowing the scope). So,

this is bad

byte[] array = new byte[kilos or megas]
for_loop:
  use(array)

this is good.

for_loop:
  byte[] array = new byte[kilos or megas]
  use(array)

I asked this question when forum.java.sun existed, and people agreed that they would prefer declaring the array as narrow as possible.

Modern java programmer never hesitates to declare a new class or create an object. He is encouraged to do so. Java gives all the reason to do so, with its idea that "everything is an object" and cheap creation.

Furthermore, modern enterprise programming style is, when you need to execute an action, you create a special class (or even better, a framework) that will execute that simple action. Good Java IDEs, that help here. They generate tons of crap automatically, as breathing.

So, I think your objects lack the Builder or Factory pattern. It is not decent to create instances directly, by constructor. A special factory class for every your object is advised.

Now, when functional programming is coming into play, creating objects becomes even simpler. You will create objects at every step as breathing, without even noticing this. So, I expect your code will become even more "efficient" in new era

"do not do anything in your constructor. It is for initialization only"

Personally, I see no reason in the guideline. I consider it a just another method. Factoring code out is necessary only when you can share it.

share|improve this answer
    
the explanation is not well structured and hard to follow. You just go all over the place with contestable assertion that have no links to context. –  New Alexandria Oct 7 '12 at 20:51
    
The really contestable statement is "Constructors should instantiate the fields of an object and do any other initialization necessary to make the object ready to use". Tell it to the most popular contributor. –  Val Jan 11 '13 at 18:34
add comment

I'm going to go against the tide here -- in languages where everything has to be in some class AND you can't have a static class, you can run into the situation where you have an isolated bit of functionality that needs to be put somewhere and doesn't fit with anything else. Your choices are a utility class that covers various unrelated bits of functionality, or a class that does basically just this one thing.

If you have just one bit like this, then your static class is your specific class. So, either you have a constructor that does all the work, or a single function that does all the work. The advantage of doing everything in the constructor is that it happens just once, it allows you to use private fields, properties and methods without worrying about them being reused or threading. If you have a constructor/method you may be tempteded to let the parameters be passed to the single method, but if you use private fields or proprties that introduces potential threading issues.

Even with a utility class, most people won't think to have nested static classes (and that might not be possible either depending upon the language).

Basically this is a relatively understandable way of isolating the behavior from the rest of the system, within what is allowed by the language, while still allowing you to take advantage of as much of the language as possible.

Frankly I would have answered much as your contractor did, it's a minor adjustment to turn it into two calls, there's not all that much to recommend one over the other. What dis/advatanges there are, are probably more hypothetical than actual, why try to justify the decision when it doesn't matter and it probably wasn't so much decided as just the default action?

share|improve this answer
add comment

Is there ever a reason to do all an object's work in a constructor?

No.

  • A constructor shouldn't have any side-effects.
    • Anything more than private field initialization should be viewed as a side-effect.
    • A constructor with side-effects breaks the Single-Responsibilty-Principle (SRP) and runs contrary to the spirit of object-oriented-programming (OOP).
  • A constructor should be light and should never fail.
    • For instance, I always shudder when I see a try-catch block inside a constructor. A constructor should not throw exceptions or log errors.

One could reasonably question these guidelines and say, "But I don't follow these rules, and my code works fine!" To that, I would reply, "Well that may be true, until it isn't."

  • Exceptions and errors inside of a constructor are very unexpected. Unless they are told to do so, future programmers will not be inclined to surround these constructor calls with defensive code.
  • If anything fails in production, the generated stack trace may be difficult to parse. The top of the stack trace may point to the constructor call, but many things happen in the constructor, and it may not point to the actual LOC that failed.
    • I've parsed many .NET stack traces where this was the case.
share|improve this answer
add comment

I agree with AndyBursh. It seems some lack of knowledge issue.

However it's important to mention frameworks like NHibernate which require you just to code in constructor (when creating map classes). However this is not a limitation of the framework, it's just what you need. When it comes to reflection, the constructor it's the only method which will always exist (although it might be private) and this might be helpful if you have to call a method of a class dynamically.

share|improve this answer
add comment

No.

If all the work is done in the constructor it means the object was never needed in the first place. Most likely, the contractor was merely using the object for modularity. In that case, a non-instantiated class with a static method would have gained the same benefit without creating any superfluous object instances.

There is only one case in which doing all the work in an object constructor is acceptable. That is when the purpose of the object is specifically to represent a long term unique identity, rather than perform work. In such a case, the object would be kept around for reasons other than performing meaningful work. For example, a singleton instance.

share|improve this answer
add comment

I've certainly created many classes that do a lot of work to calculate something in the constructor, but the object is immutable, so it makes the results of that computation available via properties, and then never does anything else. That's normal immutability in action.

I think the weird thing here is to put code with side effects into a constructor. Building an object and throwing it away without accessing the properties or methods looks weird (but at least in this case it's pretty obvious that something else is going on).

This would be better if the function was moved to a public method, and then have an instance of the class injected, then have the for loop call the method repeatedly.

share|improve this answer
add comment

For me the rule of thumb is not to do anything in the constructor, except for initialising member variables. The first reason for that is that it helps to follow SRP principle, as usually a lengthy initialization process is an indication that the class does more than it should do, and the initialization should be done somewhere outside of the class. The second reason is that this way only the necessary parameters are passed, so you create less coupled code. A constructor with complicated initialisation usually needs parameters which are used only to construct another object, but which are not used by the original class.

share|improve this answer
add comment

For me, it is a bit of a surprise when you get an object doing much work in the constructor, so just on that I'd recommend against it (principle of least surprise).

An attempt at a good example:

I'm not sure if this usage is an anti-pattern, but in C# I've seen code which was along the lines of (somewhat made up example):

using (ImpersonationContext administrativeContext = new ImpersonationContext(ADMIN_USER))
{
    // Perform actions here under the administrator's security context
}
// We're back to "normal" privileges here

In this case, the ImpersonationContext class does all of its work in the constructor and the Dispose method. It takes advantage of the C# using statement, which is fairly well understood by C# developers, and thus guarantees that the setup which occurs in the constructor is rolled back once we're outside of the using statement, even if an exception occurs. This is probably the best usage I've seen for this (i.e. "work" in the constructor), although I'm still not sure I'd consider it "good". In this case, it's fairly self-explanatory, functional and succint.

An example of a good, and similar pattern would be the command pattern. You definitely don't execute the logic in the constructor there, but it's similar in the sense that the entire class is equivalent to a method invocation (including the parameters needed to invoke the method). In this way, method invocation information can be serialised or passed for later usage, which is immensely useful. Perhaps your contractors were attempting something similar, although I highly doubt it.

Comments on your example:

I would say it's a very definite anti-pattern. It adds nothing, goes against what's expected, and performs overly long processing in the constructor (FTP in the constructor? WTF indeed, although I guess people have been known to call web services in this fashion).

I'm struggling to find the quote, but Grady Booch's book "Object Solutions" has an anecdote about a team of C developers transitioning to C++. Apparently they had been through training, and were told by management that they absolutely had to do things "object oriented". Brought in to figure out what's wrong, the author used a code metric tool, and found that the average number of methods per class was exactly 1, and were variations of the phrase "Do It". I must say, I've never encountered this particular problem before, but apparently it can be a symptom of people being forced to create object oriented code, without really understanding how to do so, and why.

share|improve this answer
3  
+1 for the good example –  Vain Fellowman Aug 2 '12 at 14:10
    
Your good example is an instance of Resource Allocation is Initialization. It's a recommended pattern in C++ because it makes it much easier to write exception-safe code. I don't know whether that carries over to C#, though. (In this case the "resource" being allocated is administrative privileges.) –  Zack Aug 2 '12 at 23:01
    
@Zack I've never thought of it in those terms, even though I've been aware of RAII in C++. In the C# world, it's referred to as the Disposable pattern, and it's very much recommended for anything which has (typically unmanaged) resources to free up after its lifetime. The main difference to this example is that you typically don't do expensive operators in the constructor. E.g. a SqlConnection's Open() method still needs to be called after the constructor. –  Daniel B Aug 3 '12 at 6:35
add comment

It seems to me that the person doing this was trying to hack around the limitation of Java that does not allow to pass functions around as first class citizens. Whenever you have to pass a function, you have to wrap it inside a class with a method like apply or similar.

So I guess he just used a shortcut and used directly a class as a function replacement. Whenever you want to execute the function, you just instantiate the class.

It is definitely not a good pattern, at least because we are here trying to figure it out, but I guess it may be the least verbose way to do something similar to functional programming in Java.

share|improve this answer
3  
I am pretty sure this particular programmer has never heard the phrase 'functional programming'. –  Kane Aug 2 '12 at 14:56
    
I don't think that the programmer was attempting to create a closure abstraction. The Java equivalent would require an abstract method (planning for possible) polymorphism. Not evidence of that here. –  Kevin A. Naudé Aug 2 '12 at 17:38
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.