// 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 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 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 enrolledMonths = new HashSet(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 enrolledMonths = new HashSet(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) is the same object as // 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 enrolledMonths = new HashSet(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 enrolledMonths = new HashSet(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 enrolledMonths = new HashSet(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 enrolledMonths = new HashSet(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.