extract.java

// We're going to pick on Mad Service today. If you had a hand in writing this once upon a time, do not fret.
// This isn't accusation or finger-poiniting -- I see everyone's name all over this code, including my own, which 
// makes it a nice target.

// This code just needs some tenderness. Let's cook it a hot bowl of soup and play some soft Jazz.








// Step zero! Does the class name make any sense?

public MadServiceImpl {
	// ...
}

// Shishir asks: "What's a Mad?"

// Shishir could very well be the next developer to join our project (or any other project, for that matter). 
// Make it easy for him... don't use abbreviations unless they're so core to the business they're used everywhere.
// ...even then, reconsider.

// In the case of Member Appreciation Discount, it's NOT core to the business so we can't abbreviate it in code.

public MemberAppreciationDiscountServiceImpl {
	// ...
}

// That's better. Always use the domain language. IntelliJ and Eclipse type for you anyway, so what do you care?
















































// Step one... find that dirty method.

    public boolean checkEligibility(Customer customer, GymPass familyPass) {
        if (!familyPass.belongsTo(ProductGroupInternalIdentifier.familyPassProductGroups())) {
            return true;
        }

        boolean validEmployeeStatus = validateEmployeeStatus(customer.primaryAffiliation());
        boolean doesntHaveOverlappingPasses = validEmployeeStatus 
			&& transactionService.getPassOverlappingMonths(customer, familyPass.getEffectiveBeginDate(), 
				familyPass.getEffectiveEndDate()).size() == 0;
        boolean doesntHaveMad = doesntHaveOverlappingPasses && !hasMadForPassEffectiveDates(customer, familyPass);
        return doesntHaveMad && (familyPassDao.retrieveFamilyPass(customer, familyPass.getEffectiveBeginDate(),
                familyPass.getEffectiveEndDate()) == null);
    }
	
// Whew. A doozy.
// Shishir asks: "Is... uh, THIS... how you write all your domain rules?"
// I cough.











































// There's more. That method has buddies.

    private boolean validateEmployeeStatus(Affiliation affiliation) {
        if (affiliation instanceof Employee) {
            Employee employee = (Employee) affiliation;
            return new MadEligibilityValidator(addressExclusionDao, clock).validEmployeeStatus(employee.getEmployeeStatus());
        }
        return true;
    }

    public boolean hasMadForGymPassEffectiveDates(Customer customer, GymPass pass) {
        List<DateRange> dateRanges = FiscalYear.getFiscalYearsBetweenDates(pass.getEffectiveBeginDate(),
                pass.getEffectiveEndDate());
        DateTime passBeginDateWithDayOfMonthAsFirstDayOfMonth = pass.getEffectiveBeginDate().withDayOfMonth(1).toDateTimeAtStartOfDay();
        DateTime passEndDateWithDayOfMonthAsLastDayOfMonth = pass.getEffectiveEndDate().dayOfMonth().withMaximumValue().toDateTimeAtStartOfDay();

        for (DateRange dateRange : dateRanges) {
            MemberAppreciationDiscount discount = getMad(customer, dateRange);
            if (discount == null)
                continue;
            List<LocalDate> enrolledMonths = discount.getEnrolledMonthsAsDates();
            for (LocalDate enrolledLocalDate : enrolledMonths) {
                Interval overlappingInterval = new Interval(passBeginDateWithDayOfMonthAsFirstDayOfMonth, passEndDateWithDayOfMonthAsLastDayOfMonth).overlap(enrolledLocalDate.toInterval());
                if (overlappingInterval != null)
                    return true;
            }
        }
        return false;
    }

    public MemberAppreciationDiscount getMad(Customer customer, DateRange fiscalYear) {
        logger.debug(customer);
        return madDao.retrieveByCustomerAndMadYear(customer, fiscalYear);
    }

// That's a lot of method for one little tiny service. When I picked this to chop up I didn't think I had quite so much
// work ahead of me. Nice job, Steve. Sigh.

// Can you spot the pain points?










































	
	
	
	

	
// How about the fact that only one of those 3 methods is private? Even a private method can be consumed here at home 
// (in MadServiceImpl, that is) ...but once she's public we're in for a world of pain. Also, why the heck is our public method
// calling another public method in the same class? Can you tell me which is MORE public? (One of them has to be.) I have no idea.
// I've said this a few times now, but remember: It's not as if one public method calling another public method is WRONG... or even BAD.
// Enums aren't wrong or bad.
// Booleans aren't wrong or bad.
// Ifs aren't wrong or bad.
// Switches aren't wrong or bad.
// Statics aren't wrong or bad.
// Singletons aren't wrong or bad.
// Inheritance isn't wrong or bad.

// Those things aren't wrong. They're not bad. THEY'RE SCARY AS HELL AND SMELL LIKE A BUM. Maybe you'll want to do these things sometime, 
// but remember someone else has to smell that bum once you're done with it. There are plenty of smelly things about one public 
// method consuming another public method on the same class, but chances are good you just need a new kind of object.

// I'll skip a step here and tell you that's our problem in this method.
// Stop. Get a beer. Make that object.

// What else scares you about this code? You have 5 seconds to tell me! Quick!






















// Quicker!!!





















































// Time's up! Here they are:
// 1. transactionService
// 2. familyPassDao
// 3. addressExclusionDao
// 4. clock
// 5. madDao
// 6. logger (doesn't really count, since it doesn't need to be there)

// Damn. How'd we find all those out in less than 5 seconds? Were you reading the code top-to-bottom? You're a fast reader.

// How do slow readers like me have to do it?













































// ...the same way we extract one object from another (which, at the end of the day, is what we're doing here): static. 
// Temporarily mark all your offending methods with static and see what blows up. Other instance methods will show up (which you 
// then must also mark as static), and fields will show up (which you have to deal with differently).

// Sometimes I'm convinced the only reason they put 'static' in Java was to find bad code. It's pretty much useless for anything
// else. Is there some cool trick we can do with 'switch' that could justify its existance?












































// Step two... find that filthy test.

public void testShouldCheckIfCustomerHasMadForPassEffectiveDates() {
	// ...
}

// It appears to be testing 5 full scenarios, which would be decent coverage for a service method if it weren't so 
// gargantuan. Whoever wrote this test-of-tests had a lot more energy than I do.

// Service tests are a lot of work and we're programmers... we hate work!




// IMPORTANT TANGENT RANT: STEVE'S STOLEN LAWS (STARTING IN THE MIDDLE)

// POINT 17A, SUBPOINT 4: Good Programmers Are Lazy[TM]. (See: http://c2.com/cgi/wiki?LazinessImpatienceHubris)

// POINT 04R, SUBPOINT 6: Good Programmers Read ALL of c2.com. All the good books are written, and it's not a secret it's 
//                        all on c2 alrady. Plus, if there happens to be a book out there you still want to read, c2.com will
//                        probably reference it somewhere.











































// Okay, whatever. Shut up, Steve! Get to the point! Anyway.

// Let's take a look at the test, then refactor the test before we hit the code.

public void testShouldCheckIfCustomerHasMadForPassEffectiveDates() {

        Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
        final DateRange dateRange = new DateRange(2007, 2008);
        MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);

        final MemberAppreciationDiscount discount =  new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();


        GymPass longTermFamilyPass = FamilyPassFixture.createLongTermFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());
        Employee employee = new Employee();
        employee.setEmployeeStatus("Leave Of Absence");
        employee.setAffiliationType(AffiliationTypeFixture.UEM);
        Customer customer = new CustomerBuilder().affiliation(employee).toCustomer();
        customer.setPrimaryAffiliationType(AffiliationTypeFixture.UEM);
        assertFalse(madService.checkEligibility(customer, longTermFamilyPass.getPass()));

        mockery.checking(new Expectations() {
            {
                exactly(3).of(transactionServiceMock)
                    .getPassOverlappingMonths(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(new ArrayList()));

                exactly(6).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));

                one(familyPassDaoMock).retrieveFamilyPass(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(null));

            }
        });

        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2007, DateTimeConstants.NOVEMBER, 30).date());
        assertTrue(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.FEBRUARY, 28).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.MARCH, 2).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2009, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.hasMadForPassEffectiveDates(this.customer, longTermFamilyPass.getPass()));

    }
	
// Wait... did you go read c2.com yet? All of it? Really? Even the dialogues between Kent Beck, Martin Fowler and Ward Cunningham?
// (It was a live wiki, edited by the rockstars of the industry, once upon a time.)

// I don't believe you. Go back and find more to read.








































	
	
	
	
// test refactoring step #1... what's wrong with the test name?

// Should Check If Customer Has Mad For Pass Effective Dates

// Could the customer read this? does it describe intent? does it tell you about the example you, as a developer, can read 
// inside the test? does it tell you a success or a failure condition?

// Not so much. Let's re-word it.




public testCustomerShouldBeEligibleForMemberAppreciationDiscountIfItOverlapsWithAPassHeHolds() {
	// ...
}

// this, of course, becomes:
// Customer should be eligible for Member Appreciation Discount if it overlaps with a pass he holds.

// Your tests are many things:
// - They are an example of how the next developer (including yourself) should consume your code.
//
// - They are a specification. At the end of the project (heck, at the end of each iteration) you should be comfortable sending 
//   a text file with a list of test names (humanized, of course) to your customer and your customer should agree with everything 
//   you've written.
//
// - They are TESTS. A test doesn't prove anything, but it does tell you that for one very particular scenario, your code probably works.
//
// - They are CODE. You'll write bad tests as often as you'll write bad code. Since all code is bad, you'd better make darn sure your
//   tests are simple, small, and readable. Inside and out.
//
// - They are the only confidence you, as a developer, should have in your code. Whether it's an acceptance test script written by a QA
//   (manual or automated) or the world's tiniest unit test, this is your starting point. Without tests, the only assumption you can 
//   reasonably make about your code is that it doesn't work. This is why you should love your QAs. QAs are developers, and they're 
//   doing the only job that lets you sleep at night.
//
// - They are a central point of conversation. When your story fails BA volleyball, you should know exactly which test you need to 
//   check to determine where the faulty code lies. Read the test names out to the BA! S/he should know exactly what you're talking
//   about -- particularly at such a high level as a service test. Tests have nothing to do with programming. They have everything 
//   to do with the business, and I hear BAs are pretty good at that stuff.
//
//   Then show the BA your code. If s/he can't read it, you've screwed up. Code is not for computers, it's for humans.
//   Write every line of code with the next person in mind, not the computer.
//   
//   Think about that for a minute. You JUST played that story. If your BA, who knows the story inside and out, can't read your code, 
//   what makes you think the next developer (who knows nothing about the story you just played) will understand your code?
































// Seriously. Have you read c2.com yet? All of it? Don't read another blog until you've read all of c2. These days, every
// developer blog you find is just some 30-year-old Ruby-writing doofus telling you what c2 and smalltalk and lisp could have 
// told you 10 years ago. Go read it, dammit.










































// Okay, now that we have a name for our first extracted test, let's pour the cupcake batter into the tray.

     public void testCustomerShouldBeEligibleForMemberAppreciationDiscountIfItOverlapsWithAPassHeHolds() {
    //  delete this shiznoz:    
    //  Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
    //  final DateRange dateRange = new DateRange(2007, 2008);
    //  MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);
    //  final MemberAppreciationDiscount discount = new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();

        GymPass longTermFamilyPass = FamilyPassFixture.createLongTermFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());
        Employee employee = new Employee();
        employee.setEmployeeStatus("Leave Of Absence");
        employee.setAffiliationType(AffiliationTypeFixture.UEM);
        Customer customer = new CustomerBuilder().affiliation(employee).toCustomer();
        customer.setPrimaryAffiliationType(AffiliationTypeFixture.UEM);
        assertFalse(madService.checkEligibility(customer, longTermFamilyPass.getPass()));
     }

// Mmm! That was easy! All we had to do was copy everything up to the first assert. And we can delete that MemberAppreciationDiscount 
// object, since it's not used in this test. See? Things are getting cleaner already. Just in case you're not sure what I mean
// by "cleaner":

// IMPORTANT TANGENT RANT: STEVE'S STOLEN LAWS (STARTING IN THE MIDDLE)

// POINT 32X, SUBPOINT orange:    [ Deleting code == hooray! ]

// You've made a change. What's next? (That was a hint.)

















































// You know it. Run those tests, boss. If you're not sure whether you should run your tests... run them. If you change 
// something, no matter how small, run them. Keep your grass green as much of the time as you can. The society grooming committee
// will be around any day now and you don't want to be caught with a sun-scorched lawn.

// Taking another look at this test (and the corresponding code), it looks like it should be hitting a DAO or something... I'm not sure
// how it's bailing out of the called method so early. I decided to debug it -- guess what I found?

// Well, paint me orange and call me sweet! I had no idea! The code returns false because of this code:

	public boolean validEmployeeStatus(String employeeStatus) {
		// ... some other crap
		employeeStatus.equalsIgnoreCase(LEAVE_OF_ABSENCE)
		// more crap
	}
	
// I guess this line in the test was more important than I thought: employee.setEmployeeStatus("Leave Of Absence");

// Lessons learned from this one? Don't just guess at the test. Whether you're cleaning up an older test or working with code under 
// a nice clean test, make sure you understand what the test is testing. If it's not testing anything, delete it.

// Obvious examples of "not testing anything" include:
// 1) <expected> is the same object as <actual>
// 2) there are no asserts or mock expectations
// 3) there are no asserts and the developer who wrote the test didn't comment it to declare he only wanted to test a mock's expectations
// 4) the test code looks freakishly similar to the code under test
// ... I'm sure you can think of plenty others.

// Anyway. Our test is now:

	public testEmployeeShouldBeIneligibleForMemberAppreciationDiscountIfHeIsOnALeaveOfAbsence()
	
// or An Employee should be ineligible for Member Appreciation Discount if she is on a leave of absence.

// I think our client would be pleased to know we're testing this acceptance criteria so directly. Her only complaint might be that the 
// sentence isn't sexually neutral ('he or she', 's/he', or any other politically correct silliness). But I think we can live with that.














































// Okay, now onto the mocks. This is where things get fun.

    public void testShouldCheckIfCustomerHasMadForPassEffectiveDates() {
	
        // ...
        
        mockery.checking(new Expectations() {
            {
                exactly(3).of(transactionServiceMock)
                    .getPassOverlappingMonths(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(new ArrayList()));

                exactly(6).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));

                one(familyPassDaoMock).retrieveFamilyPass(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(null));
            }
        });
        
        // gym pass 1
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        // gym pass 2
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2007, DateTimeConstants.NOVEMBER, 30).date());
        assertTrue(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        // gym pass 3
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.FEBRUARY, 28).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));

        // gym pass 4
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.MARCH, 2).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2009, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.hasMadForPassEffectiveDates(this.customer, longTermFamilyPass.getPass()));

    }

// Whoa, whoa. "exactly(6)"? What in the name of backwards-paddling canoes...? If you're setting up mocks for multiple asserts, 
// you have a pretty clear indication something is wrong with your test. It's fairly obvious, but I've marked up the offending
// asserts with '// gym pass N'. These dudes need to spend a little less time together. By the time we get around to refactoring
// MaaaaaadServiceImpl, we won't have the first clue what's going on in this test if it breaks.

// You know what to do here. Cut those 4 passes, and their respective asserts, into appropriately-sized tests:

    public void testShouldCheckIfCustomerHasMadForPassEffectiveDates4() {

        Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
        final DateRange dateRange = new DateRange(2007, 2008);
        MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);

        final MemberAppreciationDiscount discount = new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();

        GymPass longTermFamilyPass = FamilyPassFixture.longTermFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());

        mockery.checking(new Expectations() {
            {
                exactly(2).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));
            }
        });

        // gym pass 4
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.MARCH, 2).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2009, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.hasMadForPassEffectiveDates(this.customer, longTermFamilyPass.getPass()));
    }

// I'm starting at the bottom, because it's the most interesting. This test is covering hasMadForPassEffectiveDates() while
// the others are covering checkEligibility(). Uh... yuck. The next time you see this, slap the person who wrote it. Hard.
// What if you're tempted to add an assert to a test that's already there? Slap yourself. Hard.

// What if the assert you're adding is for the same method, like the other 4 calls?

// Slap yourself. Really hard.

// You ARE allowed to have multiple asserts per test. But DO NOT call the object under test more than once. If you're adding 
// more than one assert to a test, ask yourself why. Every time. Chances are there's probably a smaller, simpler test you could write.




// Alright, now on to the less interesting guys:

    public void testShouldCheckIfCustomerHasMadForPassEffectiveDates1() {

        Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
        final DateRange dateRange = new DateRange(2007, 2008);
        MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);

        final MemberAppreciationDiscount discount = new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();

        GymPass longTermFamilyPass = FamilyPassFixture.createLongTermFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());

        mockery.checking(new Expectations() {
            {
                exactly(1).of(transactionServiceMock)
                    .getPassOverlappingMonths(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(new ArrayList()));

                exactly(1).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));
            }
        });

        // gym pass 1
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));
    }

    public void testShouldCheckIfCustomerHasMadForPassEffectiveDates2() {

        Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
        final DateRange dateRange = new DateRange(2007, 2008);
        MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);

        final MemberAppreciationDiscount discount = new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();

        GymPass longTermFamilyPass = FamilyPassFixture.createFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());

        mockery.checking(new Expectations() {
            {
                exactly(1).of(transactionServiceMock)
                    .getPassOverlappingMonths(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(new ArrayList()));

                exactly(2).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));

                one(familyPassDaoMock).retrieveFamilyPass(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(null));
            }
        });

        // gym pass 2
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2007, DateTimeConstants.NOVEMBER, 30).date());
        assertTrue(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));
    }

    public void testShouldCheckIfCustomerHasMadForPassEffectiveDates3() {

        Set<MonthEnum> enrolledMonths = new HashSet<MonthEnum>(Arrays.asList(MonthEnum.JAN, MonthEnum.FEB, MonthEnum.DEC));
        final DateRange dateRange = new DateRange(2007, 2008);
        MadYear madYear = MadYear.createForTests(enrolledMonths, dateRange);

        final MemberAppreciationDiscount discount = new MemberAppreciationDiscountBuilder().madYear(madYear).toMemberAppreciationDiscount();

        GymPass longTermFamilyPass = FamilyPassFixture.createLongTermFamilyPass("123");
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2007, DateTimeConstants.JULY, 1).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.JANUARY, 1).date());

        mockery.checking(new Expectations() {
            {
                exactly(1).of(transactionServiceMock)
                    .getPassOverlappingMonths(with(any(Customer.class)), with(any(LocalDate.class)), with(any(LocalDate.class)));
                will(returnValue(new ArrayList()));

                exactly(1).of(madDaoMock).retrieveByCustomerAndMadYear(with(any(Customer.class)), with(any(DateRange.class)));
                will(returnValue(discount));
            }
        });

        // gym pass 3
        longTermFamilyPass.getPass().setEffectiveBeginDate(new ClockForUT(2008, DateTimeConstants.FEBRUARY, 28).date());
        longTermFamilyPass.getPass().setEffectiveEndDate(new ClockForUT(2008, DateTimeConstants.AUGUST, 31).date());
        assertFalse(madService.checkEligibility(this.customer, longTermFamilyPass.getPass()));
    }
	
// These tests now have:
// - only one assert
// - only one setup
// - only one set of mock expectations (which makes the internals much clearer for the next person who will read the test)












































// Before we go back and rename those tests, let's take a minute to think about how we're going to change the service after this.
// Why was this test written like this in the first place? (It certainly did save some space, even if it was completely incomprehensible.)
// Why? Because of the mocks. People hate mocks. I hate mocks. Stubs are better (and these mocks are halfway to a life of stubbery anyway), 
// but mocks and stubs are both are lame hacks. In a perfect world, the network and the hard disk would be no slower than your processor or RAM.
// Were this the case, you might never mock anything but a disconnected 3rd party service, because... why bother? Unfortunately, the world 
// isn't perfect, and hard disks are slow. If we're going to the database, we have to mock for our own sanity.

// But what else? Mocks also tell us something about our code. For one, as Ashwin mentioned in his JMock tutorial, they tell us when code is 
// getting "chatty"... too many mocks mean objects are tightly coupled and talking too much. Objects are, more often than not, talking
// too much because one of them is doing too much work. In this case, it's our service. He's all over the place!
// - validating
// - looping
// - nesting loops! (good lord!)
// - constructing or consuming all sorts of crazy objects -- dates, times, lists, domain objects, gym passes, and customers -- oh my!
// - asking questions of other objects and acting on the answer, rather than being lazy doing as little as he can before passing the buck
//   (Like good programmers, good objects are also lazy; lazy programmers + lazy objects is lazy object-oriented zen. Have you read c2 yet?)

// Thankfully, on the surface, he's a good guy. He plays nice in conversation:

	public boolean checkEligibility(Customer customer, GymPass familyPass) {
		// ...
	}
	
// That's pretty solid. A straightforward question for him to answer: Is this customer eligible for MAD? Yes or no? A rename wouldn't 
// hurt. Say 'canCustomerGetSomeHotMemberAppreciationDiscountAction' or perhaps the concise 'isCustomerEligibleForMemberAppreciationDiscount'. 
// Neither of these tells us what the pass is for, though -- which is a shame. Maybe Java 7 will have split method invocation or named 
// parameters, but don't hold your breath. In the meantime, you should either write a Javadoc for this method or explain what the parameter 
// means in your tests. Given that we're talking about a public method, I'd prefer it if there were both. A Javadoc saves me a trip to the 
// test when I'm consuming an object elsewhere. At a minimum, rename familyPass to something which tells you what KIND of pass it is.
// By 'kind' I don't mean 'family'. Where should this pass come from? It's a dependency. It has to have some semantic meaning, so 
// call it out.

// Why don't we walk the call stack and find out what that pass is for?

	public boolean checkEligibility(Customer customer, GymPass familyPass) { }
	// leads to:
	private boolean notEligibile(Customer customer, GymPass pass) { }
	// leads to:
	private CustomerPassEligibility.EligibilityType eligibilityTypeFor(Customer customer, GymPass pass) { }
	// leads to:
	public CustomerPassEligibility eligibilityFor(Customer customer, GymPass pass) { }
	// leads to:
	public GymPassService.CustomerPassEligibility passEligibilityForCustomer(Customer customer, GymPass pass) { }

// WHOoooAAAAAA. As it turns out, we're talking about GYM PASS eligibility here! Damn. I shouldn't have to walk potential call stacks to 
// find that out, should I? Nope. We've got some responsibility in the wrong place and some pretty poor naming going on here, but this 
// code sample is ancient and I completely trust no one on this team would rush a story like this again.

// With this knowledge, let's go back to our first test and name it appropriately:

	public void testEmployeeShouldBeIneligibleForMemberAppreciationDiscountIfHeIsOnALeaveOfAbsence() { }
	// becomes
	public void testEmployeeShouldBeIneligibleForAFamilyGymPassIfHeIsOnALeaveOfAbsence() { }
	
// and the others follow like so:

	public void testEmployeeShouldBeEligibleForAFamilyGymPass   If It Overlaps The Beginning Of His Member Appreciation Discount Period() { }
	public void testEmployeeShouldBeIneligibleForAFamilyGymPass If It Doesnt Overlap His Member Appreciation Discount Period() { }
	public void testEmployeeShouldBeEligibleForAFamilyGymPass   If It Is Overlapped By His Member Appreciation Discount Period() { }
	public void testEmployeeShouldBeEligibleForAFamilyGymPass   If It Overlaps The End Of His Member Appreciation Discount Period() { }
	
// Now these are nicer tests! It's easy to tell they cover boundary conditions around the issue we're concerned about. (they were there 
// already, I just gave them names... so kudos to whoever thought of the test scenarios originally). Now they cover some good scenarios, 
// they're easy to read, they're about as concise as service tests will get, and they're named something the client would understand. 
// Cool! It would be even cooler if they were unit tests around a domain object. But that will take a little more work.










































// For the time-being, let's take a break and talk about c2 again.

// I'm shocked to see that Wikipedia's page on c2 doesn't mention C2: http://en.wikipedia.org/wiki/C2
// A bit of history, here. C2 was the world's FIRST wiki. You'd think Wikipedia, of all websites, would pay homage to the first wiki 
// ever EVER.

// The first person to edit the Wikipedia page and properly reference c2.com gets beer and cake.
// ...Learn about it here: http://www.aboutus.org/C2.com
// ...now go edit the Wikipedia page.
//
// Done?
//
// Good. Now get back to reading c2.

// Ha ha! Just kidding. Finish this document first, then read all of c2. And even before you get back to this document, send me a text:
// +91 99 23700661 -- tell me you've updated Wikipedia and, if you're the first, you get a SERIOUS prize. Like, a way awesome prize. 
// You have no idea how crazy great it will be, but it will be DARN GREAT.















































// Okay, what's next? Well, the next step would be to replace the constructors and fixtures with builders. But before you do this, 
// ask yourself: What's the purpose of a builder? Why do we prefer it over direct construction or a fixture? Because Steve says
// builders are cool? Or is there something else? Surely there must be a good reason... and perhaps a good reason to treat these tests as 
// "good enough" for now, until the code is fixed up. If you can't see why (and to a large extent, I *hope* you have questions 
// at this point)... feel free to bring it up with me. Understand why you do the things you do. If you're doing something 
// during the workday, you should have a reason you believe in. At the worst, that reason should be this:

// "The team disagreed and we had to choose one of two options for consistency."

// But you should still understand both sides of the argument and form your own opinion, even if you appreciate others'.







// Now that we can get back to the code, let's cut out some domain objects, push this interface back to GymPassService (where it 
// belongs) and keep these 5 tests green the entire time. This is left as an exercise for the reader.