--On Friday, October 01, 2004 8:10 AM -1000 Hongbing Kou <[EMAIL PROTECTED]> wrote:
I think it is an crosscutting issue not just quality. Remember I had long list test
failures yesterday after I changed
Day.toString() so that the format in chart will be 05-Sep-2004 not 2004-09-05. This
small change affected sesor data loading such that most tests started to fail because
our code depends on toString() at many places, which is not right from my point of
view. It's the same in hackySQI: expected:<2003-01-02> but was:<02-Jan-2003>
I have to confess: I was _very_ nervous when I saw that you had made this change to the Day class! But then I figured, well, if all the test cases pass, then I guess everything's OK. :-)
Changing the toString() method of any object in the system should not break the system, I think. The tests may fail because different
content is printed out in the console or displayed in the chart, web page. In case tests breaks after we change toString() we should go
to fix unit tests.
In addition, I wondered why Hongbing was making a fundamental change to the Day class, a change in hackyKernel that affects the entire system, in order to fix the labeling of a chart for hackyReport? This seems quite bogus to me---what happens when hackyFoo wants a Day instance to be displayed as "The Seventh Day in the Month of February". Should hackyFoo feel free to unilaterally change the toString() method of Day, and thus break everyone else's code?
hackyFoo should define another object called FooDay or other name which extends Day. FooDay.toString() can just print out whatever to display. To us we want the day being displayed differently than before in our charts so I think changing Day.toString() is valid.
The correct resolution IMHO to the hackyReport chart labeling problem is to get hackyReport to depend upon something other than the toString() method of the Day class. If it's the case that JFreeChart hardwires the call to toString, then a localized approach to solving the problem with no ripple effect is to create a new wrapper class inside hackyReport called ChartDayLabel that accepts a Day instance in its constructor and has a single method called toString() that looks at the internal Day instance and returns the string formatted whatever way you want it. There won't be millions of ChartDayLabels around, so the performance impact of this wrapper should be minimal.
JFreeChart only enforces category implementing comparable interface that makes sense to them, which means client has control what to be displayed in the chart without implementing another interface.
I am not sure where to go from here. Hongbing notes above that "our code depends on toString() at many places, which is not right from my point of view", yet he has just made a change to the Day class because hackyReport depends upon the toString() method! That seems inconsistent to me.
Yes, it is controversial. My feeling is that we are free to use toString() for logging, display purpose but not logic, Ex concatenating Day.toString() + ".xml" together to make data file name. In TDD toString() is not necessary to be tested and Joy's extreme coverage excludes toString().
On the other hand, is it worth it to revert these changes now that they've been made? I mean, I do agree with Hongbing that people shouldn't depend upon toString() for important processing, although I don't see any reason why test cases can't use it, and if they fail because of a change, such as in Aaron's case, the fix is trivial.
I think so too. When I did TDD I tested toString() I just want to have good feeling on 100% coverage not because it is important. So far I have not seen a simple solution to do it in hackyReport or JFreeChart. If you think it is important to have a warper on Day object for labeling in hackyReport I can spend some time to see how I can make it happen.
Thanks a lot, Hongbing
Cheers, Philip
