Tuesday, January 30, 2007

Constructors vs. factory methods

I have come up with some coding conventions that I seem to spend a not-insignificant amount of time explaining to some of the people I work with. Specifically, I have a convention for what actions are performed in a class's constructor vs. a factory method that creates an instance of the same.



Constructors. It's not very easy to figure out what's going on if your constructor manages to throw an exception. Furthermore, if you've derived from a class whose constructor can throw exceptions, it's even more difficult to debug. Constructors should just initialize a class's members, but do no real 'work'.



Take the .NET class System.DirectoryServices.DirectoryEntry. You can construct an instance of this class with a completely bogus LDAP path in the correct format, but the constructor will not throw an exception. It is not until an action is performed using the instance that anything will go wrong.



Factory methods. In my convention, if you want to create a new instance of a physical object like an object in Active Directory, you'd have to use a factory method. If I had written DirectoryEntry, it would have a static Create() method on it, rather than having to add to the children collection, which I find a bit weird.



eg. DirectoryEntry.Create( DirectoryEntry parentContainer, string commonName, string className );



If you have two constructors, one which just encapsulates an existing physical thing, and another which creates a physical thing, that just doesn't feel right to me. Obviously this is comes down to personal style of coding, and there is nothing technically wrong with doing work in contstructors, I find that I have written more intelligible, easily readable code by clearly differentiating between the two.



My inspiration for this convention came from a Magritte painting, "The Treachery of Images". The painting, just as it says, is not a pipe. It is merely an abstract representation of a pipe. Just like the DirectoryEntry class is not actually a directory entry; it is merely an abstract representation of an object on a server that you can manipulate via code. To construct a instance of a DirectoryEntry class, you'll first need an actual existing directory entry. Let your factory method create that entry, and then return an instance of the class wrapping around it.



Am I babbling yet?

9 comments:

Tanya Cameron said...

Love the art reference ;)

Boost Ventilator said...

The Mario Version

MyDarkSecret said...

Eeesh. Where to start? Static methods?! Steve, say it ain't so! Static methods are evil. period. They break object oriented principles and should be avoided as much as the Singleton pattern. You should consider using Dependency Injection (Strategy Pattern) to avoid all of these issues. Also, I assume you aren't implying that a static method represents a Factory Pattern? A factory is different. If you have a proper exception handling structure you don't have to worry about your constructors throwing exceptions (sounds like you are treating exceptions in a procedural manner vs. an OO manner). Sounds like a C++ specific issue? Python and most other OO languages allow you to throw an exception from a constructor without issue and not have to revert to Lazy Initialization techniques.



Or am I babbling now?

Steve Dinn said...

Come on now. There is absolutely nothing wrong with static methods. In fact, FxCop, the strictest code standards enforcer I've ever used, complains if a method doesn't use "this" and isn't static.



Pretty much everything I'm going to talk about on this blog is C# related. I know that I could throw an exception from a constructor, and it isn't technically "wrong" (I said as much in the actual post), but that kind of behaviour is very non-intuitive to me.



To each their own, I guess.

MyDarkSecret said...

In very limited situations (like bootstrapping) it's ok to use static methods. To use them otherwise you are breaking object oriented principles. You break inheritance, can't overload the method and tightly couple the code. That's more than enough justification IMHO. I won't quote chapter and verse, just google "static methods are evil" ... you'll find lots of discussion about it (including similar rants about Singletons). Look into dependency injection and see the beauty of the alternatives.



But yes, to each their own I guess.



:-)

MyDarkSecret said...

Oh ... and FxCop sounds brain-dead.



The justification for requiring "this" is to prevent shadowed variables (the same as in Java):



void foo(int x) {

this.x = x;

}



Without the "this." you have a shadowed variable (which leads to Hungarian notation like m_ or _ on member variables). But, you can turn on a compiler option to always complain about shadow variables. So then you don't have to mangle your code or clutter it up with BS "this." prefixes.



Personally, I don't think there is anything more disgusting in code than having to prefix all your member variable references with "this." You end up with junior developers just mindlessly doing it without understanding the rationale (or worse, IDE's adding it automagically).



Sounds like a good beer debate.

Steve Dinn said...

Ok, I see I should have been more specific. FxCop does not look for the string "this", it looks for the use of any non-static instance methods or members. The error/warning it gives you states that "'this' wasn't used", but it doesn't mean the actual characters "this".

MyDarkSecret said...

Hmm, I don't know what that means. Seems like an off-line conversation.

Windows 7 Key said...

Absolutely brilliant post guys, been following your blog for 3 days now and i should say i am starting to like your post. and now how do i subscribe to your blog?
windows 7

Contributors