Is this a code smell? Writing code to support testing

gistfile1.as

The following two methods use the currentDateTime() method to get the current time,
rather than calling new Date() directly. the only reason I did this was so that I 
could override currentDateTime() in my test case class in order to pass in an 
existing Date object to ensure that the time would not have changed between the 
method being called and the assert made.

Is this bad practice? I can't decide.

 /**
  * Callback method used by _macroSubstitutions to provide the current date in DD_MMM_YYYY format
  */
 protected function generateDate():String
 {
    var date:Date = currentDateTime;
    var formatter:DateFormatter = new DateFormatter();
    formatter.formatString = "DD_MMM_YYYY";
    return formatter.format(date);
 }
		
 /**
  * Callback method used by _macroSubstitutions to provide the current time in HH_MM format
  */
 protected function generateTime():String
 {
   var date:Date = currentDateTime;
   var formatter:DateFormatter = new DateFormatter();
   formatter.formatString = "HH_NN";
   return formatter.format(date);
 }
		
 protected function get currentDateTime():Date
 {
   return new Date();
 }

Since you are not refactoring a legacy codebase, but rather designing your code from scratch, how about avoiding the use of class (implementation) inheritance with a simpler approach (IMHO) described by Nat Pryce and Steve Freeman in Growing Object Oriented Software Guided by Tests (http://www.growing-object-oriented-software.com/).

They say that using 'new Date()' is effectively using a singleton, i.e. a global variable. That's because "Under the covers, the constructor [in 'new Date()'] calls the singleton System and sets the new instance [of Date] to the current time using System.currentTimeMillis()".

They say that "One goal of OO as a technique for structuring code is to make the boundaries of an object clearly visible. An object should only deal with values and instances that are either local - created and managed within its scope - or passed in explicitly".

Just because you hide a dependency from the caller of a component by using a global value to bypass encapsulation, that doesn't make the dependency go away - it just makes it inaccessible (e.g. to your tests).

"To make testing easier, we need to control how Date objects are created, so we introduce a Clock and pass it into the" [class under test]

If you do that, instead of using 'new Date()', your generateDate() and generateTime() methods call clock.new(), and your test class can do the following

Arrange

ClassUnderTest cut = new ClassUnderTest(stubClock)
stubClock.setDate(TODAY)

Act

String result = cut.generateTime()

Assert

assertEquals(...,result)

David,

you said: "TDD fans: is designing code to simply facilitate easy testing a bad thing?"

It depends. See Michael Feathers' talk called 'The Deep Synergy Between Good Design and Testability' http://vimeo.com/15007792 . In it, he says (my pedestrian transcript of some excerpts follows):

KEY THESIS: solving design problems solves testing problems
BUT: making code testable does not necessarily make the design better
so there is this conflict between testability and good design
it does have a direction to it: making design better tends to make things more testable, but there are ways of making the thing more testable that aren't necessarily adherent to good design principles (e.g. the SOLID principles)

You may also find this post useful: http://michaelfeathers.typepad.com/michael_feathers_blog/2007/09/the-deep-synerg.html

Hi David,

the standard technique for doing this is to have a Clock service that is passed in as a collaborator. For instance

interface Clock { Date now(); }
class RealClock { 
    public Date now() { return new Date() }
}
class FakeClock {
    private Date toReturn;
    public FakeClock(Date toReturn) { this.toReturn = toReturn; }
    public Date now() { return toReturn; }
}
class MacroSubstitutions {
    // constructor that takes a Clock service, used by the tests
    public MacroSubstitutions(Clock clock) { this.clock = clock; }
    // default constructor, used in production code
    public MacroSubstitutions() { this(new RealClock()); }
    // ...
}

Extract-and-override is almost always a step towards introducing a new Collaborator. See "Replace Inheritance with Delegation" in Fowler's Refactoring. Introducing the Collaborator inverts the dependency, increasing context independence by pushing dependency on the runtime environment up a level of the call stack. That all sounds good to me.