Calling methods from the constructor
I wrote the code below, and as you can see, in the constructor I call some methods to perform certain operations. And now what I'm asking is, is there a good practice to call these methods from the constructor OR declare these methods as public and instantiate the object from the class information and let the object call those methods? What's a good practice for this?
Code:
class Info {
public RoadInfo(String cityName, double lat, double lng) throws FileNotFoundException, SAXException, IOException, XPathExpressionException {
// TODO Auto-generated constructor stub
this.cityname = cityName;
this.lat = lat;
this.lng = lng;
this.path = "c:"+File.separatorChar+this.cityname+".xml";
System.out.println(path);
this.initXPath();
this.method1()
this.method2()
..
this.expr = "//node[@lat='"+this.lat+"'"+"]/following-sibling::tag[1]/@v";
this.xPath.compile(this.expr);
String s = (String) this.xPath.evaluate(this.expr, this.document, XPathConstants.STRING);
System.out.println(s);
}
source to share
TL; DR In my opinion, using methods inside a constructor is a sign of bad design. If you're not looking for design tips, then the answer "no, there is nothing wrong with that, technically, as long as you avoid using unconfigured methods" should do your best. If you're looking for design tips, see below.
I think your example code is not good practice. In my opinion, a constructor should only receive values that are relevant to it, and there is no need to do any other initialization of those values. There is no way to test that your constructor "works" with all these little extra steps - all you can do is build the object and hope that everything ends up in the correct state. Also, your constructor ends up with more than one change cause that violates SRP.
class Info {
public RoadInfo(String cityName, double lat, double lng) throws FileNotFoundException, SAXException, IOException, XPathExpressionException {
// TODO Auto-generated constructor stub
this.cityname = cityName;
this.lat = lat;
this.lng = lng;
this.path = "c:"+File.separatorChar+this.cityname+".xml";
System.out.println(path);
this.initXPath();
this.method1()
this.method2()
..
this.expr = "//node[@lat='"+this.lat+"'"+"]/following-sibling::tag[1]/@v";
this.xPath.compile(this.expr);
String s = (String) this.xPath.evaluate(this.expr, this.document, XPathConstants.STRING);
System.out.println(s);
}
So, for example, this constructor loads a file by parsing it into XPath .. If I ever want to create an object RoadInfo
, now I can only do so by loading the files and having to worry about the exceptions. unit test, because now you cannot test the method this.initXPath()
separately, for example - if this.initXPath()
, this.method1()
or this.method2()
have any failures, then each of your test cases will fail. Poorly!
I would prefer it to look something like this:
class RoadInfoFactory {
public RoadInfo getRoadInfo(String cityName, double lat, double lng) {
String path = this.buildPathForCityName(cityName);
String expression = this.buildExpressionForLatitute(lat);
XPath xpath = this.initializeXPath();
XDocument document = ...;
String s = (String) xpath.evaluate(expression, document, XPathConstants.STRING);
// Or whatever you do with it..
return new RoadInfo(s);
}
}
Never mind that you have at least 5 responsibilities here.
- Build OS-neutral path
- Building an XPath Expression for Latitude / Longitude
- Create XPath Documentation
- Get it
s
- whatever it is - Create a new instance
RoadInfo
Each of these responsibilities (except the last one) should be divided into their own classes (IMO) and have to RoadInfoFactory
organize them all together.
source to share
The purpose of a constructor is to set class invariants, that is, to put a newly created object into a state that then allows clients to use them. In general, it is bad practice for an object to rely on additional initialization after it has built it. What you want to avoid is to write things in the documentation like:
... after instantiating the class
X
, remember to ALWAYS callinitX()
, or there will be bad things!
Although in some cases this is difficult to avoid and the constructor can get quite messy. For example, loading external files in the constructor is problematic.
In these cases, you can do two things:
- Rewrite your constructor to require the contents of the file instead of the name. Let the caller download. The main difference is that you require the caller to do something before the object has been created and you can express it with your constructor's signature:
public RoadInfo(String cityName, Document cityDatabase, double lat, double lng) {...}
Of course you can go even further and require a values
and let the caller do the lookup XPath. Note that all of these steps move the class to the same responsibility, which is considered a good thing. - But now you need the caller to complete many steps before they can create yours
RoadInfo
. You can use factories here that do this extra initialization as well and return fully constructed objectsRoadInfo
.
Most importantly, however, the constructor should not call any method on the object under construction that can be overridden . Calling private methods is fine, calling public methods is this
not a good idea unless the methods or the class itself are marked as final
.
If you call a method like this, there is always a chance that the class that overrides the method is doing something that breaks your functionality, such as exposing it this
to the outside world before construction is complete. Here's an example:
public abstract class Foo {
public Foo(String param) {
if (this.processParam(param) == null)
throw new IllegalArgumentException( "Can't process param.");
}
protected abstract processParam(String param);
}
public class Bar extends Foo {
public Bar(String param) {super(param);}
protected processParam(String param) {
SomeOtherClass.registerListener(this); // Things go horribly wrong here
return null;
}
}
If you now call new Bar("x")
, the constructor Foo
throws an exception because it considers this parameter to be invalid. But a Bar.processParam()
link this
to leaked SomeOtherClass
, potentially allowing SomeOtherClass
an instance to be used Bar
that shouldn't even exist.
source to share
More typically, classes that require a lot of initialization will be exposed to the client using a factory method. Constructors are often too restrictive, an accidental example is the inability to surround the initial call super
or this
using try-catch
.
If you provide a public factory method, you can make the constructor private. The constructor can only do light work like assigning final fields, and the factory takes over. This is ultimately a more robust design. Many public libraries have had to break their earlier API to implement factories that allow their code to be extended.
source to share
Is it okay to use some methods from the constructor?
Unfortunately, the only good answer to this question is: it depends on the object.
If the object is meant to hold information , then the answer should probably be no, try to avoid that, because the object should really only do one thing .
If, however, the object exists to execute a function , then be sure to make sure it is ready to execute that function by calling methods, etc. If, for example, it is a database connection, then you might want to connect to the database at build time, or at least register yourself in the connection pool.
It is good practice, however, to postpone any potentially slow stuff that you may want to postpone until you need it. In my example database, you can defer the actual connection to the database, but you will make sure to register the connection in the connection pool.
Unfortunately - the answer to another question:
Is it bad practice to call some methods from the constructor?
Also depends on the object for the same reasons.
source to share
There is no good practice, just bad practice that you shouldn't do.
When you call a method inside a constructor, some dangerous ones here:
1) a method can be overwritten and its subclassing break breaks a constructor-protected constraint of your class, an implementation out of your control
class T {
int v;
T() {
v = getValue();
}
int getValue() {
return 1;
}
}
class Sub extends T {
@Override
int getValue() {
return -1;
}
}
here T v is supposed to be 1 when you call new T()
, but when you create new Sub()
, 'v' will be set to -1, which could violate the T constraint, and this happens unconsciously.
2) a semi-constructed object has leaked, while its status may be illegal.
class T {
int a, b;
T(C c) {
// status of "this" is illegal now, but visible to c
c.calc(this);
a = 1;
b = 2;
}
}
class C {
int calc(T t) {
return t.a / t.b;
}
}
3) something more that I don't know ...
if you can prevent them all, you can do what you want.
source to share
- (Try not to throw exceptions, so the constructor can initialize the field nicely.)
- Don't call overridden methods in the constructor.
About the errors of calling the overridden method in the constructor:
Constructor (child) evaluation:
- "Zero" all fields (
0, null, 0.0, false
) - Super constructor call (implicit if not code)
- Call all field initializations (field declarations with = ...)
- The rest of the constructor code
So:
class A {
A() { f(); }
protected void f() { }
}
class B implements A {
String a;
String b = null;
String c = "c";
B() {
//[ a = null, b = null, c = null; ]
//[ super();
// B.f();
//]
//[ b = null; ]
//[ c = "c"; ]
// "nullA" - null - "c"
System.out.printf("end %s - %s - %s%n", a, b, c);
}
@Override
protected void f() {
System.out.printf("enter f : %s - %s - %s%n", a, b, c);
// null - null - null
a += "A";
b += "B";
c += "C";
// "nullA" - "nullB" - "nullC"
System.out.printf("leave f : %s - %s - %s%n", a, b, c);
}
}
This behavior is pretty confusing, and assignments are made here that are immediately overwritten by field initialization.
A common call often seen in constructors is a setter, which may have some normalization code. Make this setter public final void setX(X x);
.
source to share