November 05, 2004Why chaining constructors is badI personally find that this code snippet demonstrating constructor chaining is hard to read. This code makes it very hard to identify which of the many constructors is the "real" one: the constructor that does the real job and that I should call if I add a new overloaded constructor. This is why I use the following two rules when I write overloaded constructors:
Here is an example, from worst:
public class Person {
private String m_firstName = null;
private String m_lastName = null;
private long m_ssn = 0;
public Person(String lastName) {
m_firstName = null;
m_lastName = lastName;
m_ssn = getSsn();
}
public Person(String firstName, String lastName) {
m_firstName = firstName;
m_lastName = lastName;
}
...
to better:
public Person(String lastName) {
this(null, lastName);
m_ssn = getSsn();
}
public Person(String firstName, String lastName) {
m_firstName = firstName;
m_lastName = lastName;
}
... to best:
public Person(String lastName) {
init(null, lastName);
}
public Person(String firstName, String lastName) {
init(firstName, lastName);
}
private void init(String firstName, String lastName) {
m_firstName = null;
m_lastName = lastName;
if (null == m_firstName) {
m_ssn = getSsn();
}
}
Here is why the latter form is preferable:
Posted by cedric at November 5, 2004 10:04 AM Comments
Declaring members 'final' is a useful way to force initialization at construction time: private final String m_lastName; public Person(String lastName) { Doesn't the 'init()' construct prevent such a declaration ? Posted by: Luc Claes at November 5, 2004 10:30 AMAs Luc, I loves final member so that I know I don't need to worry about them later on ;-) The problem I have this is that there is a secret Object() constructor called before each of the init() calls in the 3rd example. Now, this may be fine for classes which subclass Object. What about when you have superclasses which don't define a no-op constructor? I despise the hidden constructor calls, and always make the call explicit (ie, add a super(); before the init() calls). I also despise hidden no-arg constructors, and always add one explicitly. Too much magic. Posted by: at November 5, 2004 10:49 AMYes it does! Here is what I responded to Luc: > Doesn't the 'init()' construct prevent such a declaration ? No, if you don't initialize your field in the declaration, you can assign it later: private final String m_firstName; private void init() { Making them final guarantees they are only initialized once, but you can do it anywhere. Besides, there are times where you can't initialize your class at construction time, but that's a different debate. -- Luc is correct... public class Person public Person(String lastName) public Person(String firstName, String lastName) private void init(String firstName, String lastName) private String getSsn() Compile your suggested code and you will get the following errors: Information: 3 errors BTW, the original source that you referenced, does support final members and almost satisfies your requirements: I'm not sure I'm totally correct :) public class FinalTest { But is Eclipse right or too conservative ? I stand corrected, it's not possible to assign a final value outside of the constructor... serves me right for not trying it. Tim is right, if you have final values, you need to pick one constructor to be the canonical one, but if you have no final variables, I think init() is easier to locate when you browse your source. Perhaps not central to your main point, but using a null firstname to indicate "init ssn" is poor style as well... Posted by: Jonathan Ellis at November 5, 2004 01:04 PMHow is reading the name of a constructor easier than reading the name of a method? It's not hard to scroll down until you see a bunch of code. If you are going to create a non-compiler-enforced convention, why not make it one that specifies the "doit" constructor is the first thing declared in the class? Of course, all this wouldn't be necessary if you could specificy default values a la C++ Posted by: a at November 5, 2004 01:09 PMHow is reading the name of a constructor harder than reading the name of a method? It's not hard to scroll down until you see a bunch of code. If you are going to create a non-compiler-enforced convention, why not make it one that specifies the "doit" constructor is the first thing declared in the class? Of course, all this wouldn't be necessary if you could specificy default values a la C++ Posted by: a at November 5, 2004 01:09 PMsorry for double post, caught a logic-reversal bug after I hit submit. Second snide comment is the intended one :) Posted by: a at November 5, 2004 01:10 PMBecause the name of the constructor changes for every class, while init() can be kept constant throughout your code base. Seems to me that a standardized comment ("this is the init constructor") give you the same scannability for the "Real" constructor. Maybe even a custom javadoc tag to identify the "full constructor" in the api-docs? @fullconstructor or something... Posted by: Rob Meyer at November 5, 2004 03:03 PMI don't think that having an init() method is always a good idea, since this method may unintentionally be called in places OTHER than the constructor. Initialization code should be put in the constructor. If there is common code that is used when the class is first initialized as well as later on (ie, it has the notion of being "reinitialized") then having a "reInitialize()" type of method this is called from the constructor (and potentially other places) is desirable. Posted by: Ryan LeCompte at November 5, 2004 03:50 PMYou could also create an annotation so you could declare private @Initializer MyClass(lots of, args here) {..} Posted by: Keith Lea at November 5, 2004 04:31 PMHmmm ¿que? ;-) Your code is broken ;-) (or I am having a space-cadet moment...) Your init method implementation is not equivalent to the other two. As far as I can see, it will never set firstname to anything other than null.. And even if "m_firstName = null" should be "m_firstName = firstName", its still not equivalent to either example above it. calling new Person(null, "moo") on the constructor examples will give different results than your init method. Ultimately, your init method code is exactly the code that *should* be in your chained constructor... The code should look like: public Person(String lastName) { public Person(String firstName, String lastName){
if (null == m_firstName) { Making the "=" instead of "==" error cant happen in Java... -Nick Posted by: Nick Minutello at November 5, 2004 05:20 PMOk, i will join this one, because i was quoted as bad example ;-) First of all, i like init() methods and i use them all over the time, but they have some risks. Its easy to call them more than once when you do inheritance. public class Person extends Human { private void init() { public class Human { public Human () { private void init() { What i mean is, you can easily come into trouble when you dont chain the init() methods correctly. And i think that my example is really not hard to read, you have a lot of this-constructors and one with some code. But technically these two ways are not too far away, both try to focus on a certain place where all the stuff happens. Marc Posted by: Marc Logemann at November 6, 2004 12:25 AMNote: to prevent comments like "why do you use super() in Person()" --> i just wanted to outline the callgraph, for the case that perhaps 2% of the visitors dont know about the auto-insertion of super() :-) Posted by: Marc Logemann at November 6, 2004 12:47 AMWhenever I have more than one constructor, one of them is called by all the others, as in the JTextField example you linked to. That approach is clear and avoids redundancy. Besides, my classes rarely have more than 2 or 3 constructors, keeping them from being that confusing. Posted by: Julian at November 7, 2004 11:27 AMWhen I design a class, I always try to apply the following rule : Constructors must not invoke overridable methods, directly or...indirectly ! Having said that, since Person class is not final, I'm assuming the getSsn() method in your example should not be overridden ?! Cedric, you are right that I did elide your point of finding the privileged constructor with a special name. Would you find my counter-example if the "real" constructor was listed first and all the others following? I am a strong believer the value of clarity, so I take your point quite seriously. Posted by: B. K. Oxley (binkley) at November 8, 2004 05:08 PMI'm with what seems to be the majority of the crowd and would prefer to call a chained constructor. I agree with you that the body of this constructor should be as expressive as your init method, i.e. it should communicate clearly under which circumstances we need to initialize the SSN. I'd also avoid long chains of constructor calls, i.e. calling a single other constructor should suffice in almost all cases. Cheers, The best is to make the variables final (which is impossible using the init-method): public class Person { public Person(String lastName) { public Person(String firstName, String lastName) { It is standard practice to have one constructor - the one with the most parameters - do ALL the work, with all other constructors calling that one constructor. Since this is the *standard*, it is also the most readable because most programmers reading your code will *expect* this pattern. I find it is rarely the case that values I initialize in constructors can be declared as "final". Sometimes this may be the case, but many times the values might change over the course of time. Posted by: Joe Maia at November 10, 2004 11:25 PMIt is standard practice to have one constructor - the one with the most parameters - do ALL the work, with all other constructors calling that one constructor. Since this is the *standard*, it is also the most readable because most programmers reading your code will *expect* this pattern. I find it is rarely the case that values I initialize in constructors can be declared as "final". Sometimes this may be the case, but many times the values might change over the course of time. Post a comment
|