Is it considered bad design to do lengthy operations in a constructor?

Question

I am implementing a class to compare directory trees (in C#). At first I implemented the actual comparison in the class's constructor. Like this:

DirectoryComparer c = new DirectoryComparer("C:\\Dir1", "C:\\Dir2");

But it doesn't feel "right" to do a possible lengthy operation in the constructor. An alternative way is to make the constructor private and add a static method like this:

DirectoryComparer c = DirectoryComparer.Compare("C:\\Dir1", "C:\\Dir2");

What do you think? Do you expect a constructor to be "quick"? Is the second example better or is it just complicating the usage of the class?

BTW:

I wont mark any answer as accepted because I don't think there is a correct answer, just preference and taste.

Edit:

Just to clarify my example a little. I'm not only insterested if the directories differs, I'm also interested in how they differ (which files). So a simple int return value wont be enough. The answer by cdragon76.myopenid.com actually is pretty close to what I want (+1 to you).

Answer 1

I prefer the second one.

I expect the constructor to instanciate the class. The method compare does what it is designed to do.

Answer 2

You should never do anything that might fail in a constructor. You don't want to ever create invalid objects. While you could implement a "zombie" state where the object doesn't do much, it's much better to perform any complex logic in seperate methods.

Answer 3

Yes, typically a constructor is something quick, it is designed to prepare the object for use, not to actually do operations. I like your second option as it keeps it a one line operation.

You could also make it a bit easier by allowing the constructor to pass the two paths, then have a Compare() method that actually does the processing.

Answer 4

I think an interface might be what you're after. I would create a class to represent a directory, and have that implement the DirectoryComparer interface. That interface would include the compare method. If C# already has a Comparable interface, you could also just implement that.

In code, your call would be:

D1 = new Directory("C:\");
..
D1.compare(D2);

Answer 5

I like the second example because it explains what is exactly happening when you instantiate the object. Plus, I always use the constructor to initialize all of the global settings fro the class.

Answer 6

I would think a combination of the two is the "right" choice, as I would expect the Compare method to return the comparison result, not the comparer itself.

DirectoryComparer c = new DirectoryComparer();

int equality = c.Compare("C:\\Dir1", "C:\\Dir2");

...and as Dana mentions, there is an IComparer interface in .Net that reflects this pattern.

The IComparer.Compare method returns an int since the use of IComparer classes is primarily with sorting. The general pattern though fits the problem of the question in that:

1. Constructor initializes an instance with (optionally) "configuring" parameters
2. Compare method takes two "data" parameters, compares them and returns a "result"

Now, the result can be an int, a bool, a collection of diffs. Whatever fits the need.

Answer 7

If you are working with C#, you could use extension methods to create a method for comparing 2 directories that you would attach to the build in DirectoryClass, so it would look some thing like:

Directory dir1 = new Directory("C:\.....");
Directory dir2 = new Directory("D:\.....");

DirectoryCompare c = dir1.CompareTo(dir2);

This would be much clearer implementation. More on extension methods here.

Answer 8

If an operation may take an unknown amount of time, it is an operation you might want to export into a different thread (so your main thread won't block and can do other things, like showing a spinning progress indicator for example). Other apps may not want to do this, they may want everything within a single thread (e.g. those that have no UI). Moving object creation to a separate thread is a bit awkward IMHO. I'd prefer to create the object (quickly) in my current thread and then just let a method of it run within another thread and once the method finished running, the other thread can die and I can grab the result of this method in my current thread by using another method of the object before dumping the object, since I'm happy as soon as I know the result (or keeping a copy if the result involves more details I may have to consume one at a time).

Answer 9

I agree with the general sentiment of not doing lengthy operations inside constructors.

Additionally, while on the subject of design, I'd consider changing your 2nd example so that the DirectoryComparer.Compare method returns something other than a DirectoryComparer object. (Perhaps a new class called DirectoryDifferences or DirectoryComparisonResult.) An object of type DirectoryComparer sounds like an object you would use to compare directories as opposed to an object that represents the differences between a pair of directories.

Then if you want to define different ways of comparing directories (such as ignoring timestamps, readonly attributes, empty directories, etc.) you could make those parameters you pass to the DirectoryComparer class constructor. Or, if you always want DirectoryComparer to have the exact same rules for comparing directories, you could simply make DirectoryComparer a static class.

For example:

DirectoryComparer comparer = new DirectoryComparer(
    DirectoryComparerOptions.IgnoreDirectoryAttributes
);
DirectoryComparerResult result = comparer.Compare("C:\\Dir1", "C:\\Dir2");

Answer 10

If the arguments are just going to be processed once then I don't think they belong as either constructor arguments or instance state.

If however the comparison service is going to support some kind of suspendable algorithm or you want to notify listeners when the equality state of two directories changes based on filesystem events or something like that. Then ther directories is part of the instance state.

In neither case is the constructor doing any work other than initializing an instance. In case two above the algorithm is either driven by a client, just like an Iterator for example, or it's driven by the event listening thread.

I generally try to do things like this: Don't hold state in the instance if it can be passed as arguments to service methods. Try to design the object with immutable state. Defining attributes, like those used in equals and hashcode should allways be immutable.

Conceptualy a constructor is a function mapping an object representation to the object it represents.

By the definition above Integer.valueOf(1) is actually more of a constructor than new Integer(1) because Integer.valueOf(1) == Integer.valueOf(1). , In either case this concept also means that all the cosntructor arguments, and only the constructor argument, should define the equals behavior of an object.

Answer 11

I would definitely do the second.

Long actions in a constructor are fine if they are actually building the object so it is usable.

Now one thing that I see people do in constructors is call virtual methods. This is BAD since once someone uses you as a base class and overrides one of those functions you will call the base class's version not the derived class once you get into your constructor.

Answer 12

I don't think that talking about abstract terms like "lengthy" have anything to do with the decision if you put something in an constructor or not.

A constructor is something that should be used to initialize an object, a method should be used to "do something", i.e. have a function.

Answer 13

I think it's not only okay for a constructor to take as much time as needed to construct a valid object, but the constructor is required to do so. Deferring object creation is very bad as you end up with potentially invalid objects. So, you will have to check an object everytime before you touch it (this is how it is done in the MFC, you have bool IsValid() methods everywhere).

I only see a slight difference in the two ways of creating the object. One can see the new operator as a static function of the class anyway. So, this all boils down to syntactic sugar.

What does the DirectoryComparer class do? What is it's responsibility? From my point of view (which is a C++ programmer's view) it looks like you'd be better off with just using a free function, but I don't think that you can have free functions in C#, can you? I guess you will collect the files which are different in the DirectoryComparer object. If so, you could better create something like an array of files or an equivalent class that's named accordingly.