views:

85

answers:

4

This is a design question. I'm trying to decide between 2 implementations.

In order to properly explain this I need an example. So, for the sake of example:

I have a class that generates a report about, let's say, certain Stock Market values on a specific day. I create a StockMarketReportGenerator object, passing it today's date, and it generates a report based on today's market values.

The StockMarketReportGenerator "has a" StockMarketData object. The purpose of the StockMarketData object is to contain all the stock market values that are stored in a table (probably called StockMarket :) ) in the database, and some further values calculated from the table data. It has private methods that connect with the database, retrieve the data, do the necessary calculations, and store the final values in the object's member variables. (It then has getter methods to expose these values, but no setter methods.) The StockMarketData class is basically a "holder" for stock market data values. I have one central function called something like "calculateStockMarketData()" that calls all these private helper methods and sets up the object. (I am aware that all this processing could really be more easily handled by a framework such as Hibernate; but the decision was made to do it manually as it's a very small, somewhat temporary project and not worth the setup.)

My question is this - from my ReportGenerator class, I only need the StockMarketData object in order to access it's properties / member variables - post-processing and post-calculations. Which means that really, I want to get the object pre-filled with data. And so I keep the calculateStockMarketData method private and call it automatically from the StockMarketData constructor. But I'm feeling somewhat uneasy about doing all my processing in a constructor and then not having any public methods. Is this a design flaw? Or is it really the most logical way to do this? Basically, which of the following 2 implementations is better?

1) (My current implementation) Make the central calculateStockMarketData() method private and call it from the StockMarketData method constructor (passing today's date), so that whenever you have a StockMarketData object, it's already filled. So all I would need from the ReportGenerator class before I start using the object properties is the line:

StockMarketData myData = new StockMarketData(someDate);

2) Make the central calculateStockMarketData() method public, so that in order to set up a StockMarketData object you need to explicitly call the method. So from the ReportGenerator class I would code:

StockMarketData myData = new StockMarketData(someDate);
myData.calculateStockMarketData();

The first strikes me as the better design, especially since there is then no possibility of using the object properties before they are initialized. But I am also unsure about the standards regarding executing a great deal of code from a constructor... Which should I choose?

A: 

A good practice is to not execute a lot of code inside a constructor.

Tuomas Pelkonen
+1  A: 

I would go with number 2, especially if there is a possibility of adding methods to the class that don't rely on that data being there.

On the other hand, if you would consider the class to be in an invalid state without the data populated, you should do it on construction.

SP
+1  A: 

I always load explicitly, post-construction.

Calling a database from a constructor can make for difficult debugging, more fragile, inflexible code, and cause performance problems if your usage of the object changes.

mrjoltcola
StockMarketData might not actually perform any DB operations during construction, it seems more like a DAO upon which StockMarketReportGenerator is dependent. So passing in a valid DAO reference at construction time is a good thing.
crowne
+7  A: 

Martin Fowler has written a good essay on dependency injection (Constructor vs. Setter injection). His advice is "as much as possible, to create valid objects at construction time".

IMO, It's best to construct valid objects if possible, because it makes it easier to read/observe the behavior of the code, since you can assume that the objects were constructed properly, and you have less bugs related to objects not being filled in properly. The issue of setter vs. constructor injection is different from what you are asking about though, which is a matter of where to execute your business logic. I think it is best to use the constructor for creating a valid object and then execute the actual business logic in another public method (your #2), so that the construction of the object can happen at a different time than the actual business logic.

Ken Liu
+1. There is a clear delineation between the construction of the generator and the performance of its function. "Not-Yet-Started" is a perfectly valid initial state for a report generator, so there's no need to front-load all the work in the constructor.
Noel Ang
I like the approach taken by Guice where all object dependencies are passed in once to the constructor. See http://code.google.com/p/google-guice/
crowne