My concern in the code below is that the param to constructor is not actually directly mapped to the class's instance fields. The instance fields derive value from the parameter and for which I'm using the initalize method. Further, I do some stuff so that the object created can be used directly in the code that follows e.g. calling drawBoundaries(). I feel it's doing what is meant by creating(initializing) a Canvas in an abstract sense.
Is my constructor doing too much? If I add methods to call the stuff in constructor explicitly from outside, that'll be wrong. Please let me know your views.
public class Canvas {
private int numberOfRows;
private int numberOfColumns;
private final List<Cell> listOfCells = new LinkedList<Cell>();
public Canvas(ParsedCells seedPatternCells) {
initalizeCanvas(seedPatternCells);
}
private void initalizeCanvas(ParsedCells seedPatternCells) {
setNumberOfRowsAndColumnsBasedOnSeedPatten(seedPatternCells);
drawBoundaries();
placeSeedPatternCellsOnCanvas(seedPatternCells);
}
...
P.S.: Sorry if this looks like a silly question; my code is going to be reviewed by an OOP guru and I'm just worried :-0
EDIT:
I read some concerns about the methods in initalizeCanvas() being over-ridden - luckily these methods are private and do no call any other methods.
Anyways, after further research on net I've started liking this more... I hope you guys agree !!??
public class Canvas {
private int numberOfRows;
private int numberOfColumns;
private final List<Cell> listOfCells = new LinkedList<Cell>();
private Canvas() {
}
public static Canvas newInstance(ParsedCells seedPatternCells) {
Canvas canvas = new Canvas();
canvas.setNumberOfRowsAndColumnsBasedOnSeedPatten(seedPatternCells);
canvas.drawBoundaries();
canvas.placeSeedPatternCellsOnCanvas(seedPatternCells);
return canvas;
}
It is generally a bad idea for a constructor to contain non-trivial code. As a rule, constructors should at most assign supplied values to fields. If an object requires complex initialization, that initialization should be the responsibility of another class (typically a factory). See Miško Hevery's great write-up on this topic: Flaw: Constructor does Real Work.
You should never call non-final methods in a constructor. Effective Java does a good job explaining why, but basically your object is not in a stable state before the constructor returns. If your constructor calls methods which are overridden by a subclass, you can get strange, undefined behavior.
Also see this answer.
Although its not the most elegant way to do it, I don't see it as flawed from OO perspective. However, if you are not calling the private method initalizeCanvas from anywhere else within the class, then you can consider moving those three lines to the constructor itself.
I see two potential issues:
Are the methods you call in initializeCanvas private or final? If they are not, it's possible for a subclass to override them and unwittingly break your constructor.
Are you doing graphics operations in the drawBoundaries method? It's good practice for a constructor to do only the minimum necessary to create a valid object. Are those operations necessary for the canvas to have a valid initial state?
It depends.
It is not bad for a constructor to call a private method, provided that the private method doesn't call other methods that could be overridden. However if the method or one of the methods that it calls could be overridden, you can run into trouble. Specifically the override method will be called before the overriding classes constructor runs and will see instance fields before they have been initialized.
A second problem if that the some of the methods in your initalizeCanvas method look like they my "publish" the current object before it has been fully initialized. This is potentially problematic if the application is multi-threaded, and could result in other threads seeing stale field values.
A private init function might be useful if you have multiple constructors where some parameters are defaulted or inferred. After everything is determined, one private init function fills in the object.
In a one-constructor class, probably not.
