The discussion began with @dhh making the following points on TDD and test technique, which I hope I’ve got right. Firstly, the strict definition of TDD includes the following:
- TTD is used to drive unit tests
- You can’t have collaborators
- You can’t touch the database
- You can’t touch the File system
- Fast Unit Tests, complete in the blink of an eye.
@Kent_Beck said that TDD didn’t necessarily include heavy mocking and the discussion continued...
I’ve paraphrased a little here; however, it was the difference in the interpretation and experience of using TDD that got me thinking.
Was it really a problem with TDD or was it with @dhh's experience of other developer's interpretation of TDD? I don't want to put words into @dhh's mouth, but it seems like the problem is the dogmatic application of the TDD technique even when it isn't applicable. I came away with the impression that, in certain development houses, TDD had degenerated into little more than Cargo Cult Programming.
In it, Feynman highlights experiments from several pseudosciences, such as educational science, psychology, parapsychology and physics, where the scientific approach of keeping an open mind, questioning everything and looking for flaws in your theory have been replaced by belief, ritualism and faith: a willingness to take other peoples results for granted in lieu of an experimental control.
Taken from the 1974 paper, Feynman sums up Cargo Cult Science as:
"In the South Seas there is a cargo cult of people. During the war they saw airplanes land with lots of good materials, and they want the same thing to happen now. So they've arranged to imitate things like runways, to put fires along the sides of the runways, to make a wooden hut for a man to sit in, with two wooden pieces on his head like headphones and bars of bamboo sticking out like antennas--he's the controller--and they wait for the airplanes to land. They're doing everything right. The form is perfect. It looks exactly the way it looked before. But it doesn't work. No airplanes land. So I call these things cargo cult science, because they follow all the apparent precepts and forms of scientific investigation, but they're missing something essential, because the planes don't land."
You can apply this idea to programming where you'll find teams and individuals carrying out ritualised procedures and using techniques without really understanding the theory behind them in the hope that they'll work and because they are the 'right thing to do'. In the second talk in the series @dhh came up with an example of what he called "test induced design damage” and at this I got excited because it's something I've seen a number of times. The only reservation I had about the gist code was that to me it didn't seem to result from TDD, that argument seems a little limited; I'd say that it was more a result of Cargo Cult Programming and that's because in the instances where I've come across this example TDD wasn't used.
If you've seen the Gist, you may know what I'm talking about; however, that code is in Ruby, which is something I've little experience of. In order to explore this in more detail, I thought that I'd create a Spring MVC version and go from there.
The scenario here is one where we have a very simple story: all the code does is to read an object from the database and place it into the model for display. There's no additional processing, no business logic and no calculations to perform. The agile story would go something like this:
Title: View User Details
As an admin user
I want to click on a link
So that I can verify a user's details
As an admin user
I want to click on a link
So that I can verify a user's details
In this 'Proper' N tier sample, I have a User model object, a controller and service layer and DAO together with their interfaces and tests.
And, there's the paradox: you set out to write the best code you possibly can to implement the story, using the well known and probably most popular MVC 'N' layer pattern and end up with something that's total overkill for such a simple scenario. Something, as @jhh would say, is damaged.
In my sample code, I'm using the JdbcTemplate class to retrieve a user's details from a MySQL database, but any DB access API will do.
This is the sample code demonstrating the conventional, 'right' way of implementing the story; prepare to do a lot of scrolling...
public class User {
public static User NULL_USER = new User(-1, "Not Available", "", new Date());
private final long id;
private final String name;
private final String email;
private final Date createDate;
public User(long id, String name, String email, Date createDate) {
this.id = id;
this.name = name;
this.email = email;
this.createDate = createDate;
}
public long getId() {
return id;
}
public String getName() {
return name;
}
public String getEmail() {
return email;
}
public Date getCreateDate() {
return createDate;
}
}@Controller
public class UserController {
@Autowired
private UserService userService;
@RequestMapping("/find1")
public String findUser(@RequestParam("user") String name, Model model) {
User user = userService.findUser(name);
model.addAttribute("user", user);
return "user";
}
}public interface UserService {
public abstract User findUser(String name);
}@Service
public class UserServiceImpl implements UserService {
@Autowired
private UserDao userDao;
/**
* @see com.captaindebug.cargocult.ntier.UserService#findUser(java.lang.String)
*/
@Override
public User findUser(String name) {
return userDao.findUser(name);
}
}public interface UserDao {
public abstract User findUser(String name);
}@Repository
public class UserDaoImpl implements UserDao {
private static final String FIND_USER_BY_NAME = "SELECT id, name,email,createdDate FROM Users WHERE name=?";
@Autowired
private JdbcTemplate jdbcTemplate;
/**
* @see com.captaindebug.cargocult.ntier.UserDao#findUser(java.lang.String)
*/
@Override
public User findUser(String name) {
User user;
try {
FindUserMapper rowMapper = new FindUserMapper();
user = jdbcTemplate.queryForObject(FIND_USER_BY_NAME, rowMapper, name);
} catch (EmptyResultDataAccessException e) {
user = User.NULL_USER;
}
return user;
}
}If you take a look at this code, paradoxically it looks fine; in fact it looks like a classic text book example of how to write an 'N' tier MVC application. The controller passes responsibility for sorting out the business rules to the service layer and the service layer retrieves data from the DB using a data access object, which in turn uses a RowMapper<> helper class to retrieve a User object. When the controller has a User object it injects it into the model ready for display. This pattern is clear and extensible; we're isolating the database from the service and the service from the controller by using interfaces and we're testing everything using both JUnit with Mockito, and integration tests. This should be the last word in text book MVC coding, or is it? Let's look at the code.
Firstly, there's the unnecessary use of interfaces. Some would argue that it's easy to switch database implementations, but who ever does that?1 plus, modern mocking tools can create their proxies using Class definitions so, unless your design specifically requires multiple implementations of the same interface, then using interfaces is pointless.
Next, there is the UserServiceImpl, which is a classic example of the lazy class anti-pattern, because it does nothing except pointlessly delegate to the data access object. Likewise,the controller is also pretty lazy as it delegates to the lazy UserServiceImpl before adding the resulting User class to the model: in fact, all these classes are examples of the lazy class anti pattern.
Having written some lazy classes, they are now needlessly tested to death, even the non-event UserServiceImpl class. It's only worth writing tests for classes that actually perform some logic.
public class UserControllerTest {
private static final String NAME = "Woody Allen";
private UserController instance;
@Mock
private Model model;
@Mock
private UserService userService;
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
instance = new UserController();
ReflectionTestUtils.setField(instance, "userService", userService);
}
@Test
public void testFindUser_valid_user() {
User expected = new User(0L, NAME, "[email protected]", new Date());
when(userService.findUser(NAME)).thenReturn(expected);
String result = instance.findUser(NAME, model);
assertEquals("user", result);
verify(model).addAttribute("user", expected);
}
@Test
public void testFindUser_null_user() {
when(userService.findUser(null)).thenReturn(User.NULL_USER);
String result = instance.findUser(null, model);
assertEquals("user", result);
verify(model).addAttribute("user", User.NULL_USER);
}
@Test
public void testFindUser_empty_user() {
when(userService.findUser("")).thenReturn(User.NULL_USER);
String result = instance.findUser("", model);
assertEquals("user", result);
verify(model).addAttribute("user", User.NULL_USER);
}
}public class UserServiceTest {
private static final String NAME = "Annie Hall";
private UserService instance;
@Mock
private UserDao userDao;
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
instance = new UserServiceImpl();
ReflectionTestUtils.setField(instance, "userDao", userDao);
}
@Test
public void testFindUser_valid_user() {
User expected = new User(0L, NAME, "[email protected]", new Date());
when(userDao.findUser(NAME)).thenReturn(expected);
User result = instance.findUser(NAME);
assertEquals(expected, result);
}
@Test
public void testFindUser_null_user() {
when(userDao.findUser(null)).thenReturn(User.NULL_USER);
User result = instance.findUser(null);
assertEquals(User.NULL_USER, result);
}
@Test
public void testFindUser_empty_user() {
when(userDao.findUser("")).thenReturn(User.NULL_USER);
User result = instance.findUser("");
assertEquals(User.NULL_USER, result);
}
}public class UserDaoTest {
private static final String NAME = "Woody Allen";
private UserDao instance;
@Mock
private JdbcTemplate jdbcTemplate;
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
instance = new UserDaoImpl();
ReflectionTestUtils.setField(instance, "jdbcTemplate", jdbcTemplate);
}
@SuppressWarnings({ "unchecked", "rawtypes" })
@Test
public void testFindUser_valid_user() {
User expected = new User(0L, NAME, "[email protected]", new Date());
when(jdbcTemplate.queryForObject(anyString(), (RowMapper) anyObject(), eq(NAME))).thenReturn(expected);
User result = instance.findUser(NAME);
assertEquals(expected, result);
}
@SuppressWarnings({ "unchecked", "rawtypes" })
@Test
public void testFindUser_null_user() {
when(jdbcTemplate.queryForObject(anyString(), (RowMapper) anyObject(), isNull())).thenReturn(User.NULL_USER);
User result = instance.findUser(null);
assertEquals(User.NULL_USER, result);
}
@SuppressWarnings({ "unchecked", "rawtypes" })
@Test
public void testFindUser_empty_user() {
when(jdbcTemplate.queryForObject(anyString(), (RowMapper) anyObject(), eq(""))).thenReturn(User.NULL_USER);
User result = instance.findUser("");
assertEquals(User.NULL_USER, result);
}
}@RunWith(SpringJUnit4ClassRunner.class)
@WebAppConfiguration
@ContextConfiguration({ "file:src/main/webapp/WEB-INF/spring/appServlet/servlet-context.xml",
"file:src/test/resources/test-datasource.xml" })
public class UserControllerIntTest {
@Autowired
private WebApplicationContext wac;
private MockMvc mockMvc;
/**
* @throws java.lang.Exception
*/
@Before
public void setUp() throws Exception {
mockMvc = MockMvcBuilders.webAppContextSetup(wac).build();
}
@Test
public void testFindUser_happy_flow() throws Exception {
ResultActions resultActions = mockMvc.perform(get("/find1").accept(MediaType.ALL).param("user", "Tom"));
resultActions.andExpect(status().isOk());
resultActions.andExpect(view().name("user"));
resultActions.andExpect(model().attributeExists("user"));
resultActions.andDo(print());
MvcResult result = resultActions.andReturn();
ModelAndView modelAndView = result.getModelAndView();
Map<String, Object> model = modelAndView.getModel();
User user = (User) model.get("user");
assertEquals("Tom", user.getName());
assertEquals("[email protected]", user.getEmail());
}
}In writing this sample code, I’ve added everything I could think of into the mix. You may think that this example is ‘over the top’ in its construction especially with the inclusion on redundant interface, but I have seen code like this.
The benefits of this pattern are that it follows a distinct design understood by most developers; it's clean and extensible. The down side is that there are lots of classes. More classes take more time to write and,you ever have to maintain or enhance this code, they're more difficult to get to grips with.
So, what's the solution? That's difficult to answer. In the #IsTTDDead debate @dhh gives the solution as placing all the code in one class, mixing the data access with the population of the model. If you implement this solution for our user story you still get a User class, but the number of classes you need shrinks dramatically.
@Controller
public class UserAccessor {
private static final String FIND_USER_BY_NAME = "SELECT id, name,email,createdDate FROM Users WHERE name=?";
@Autowired
private JdbcTemplate jdbcTemplate;
@RequestMapping("/find2")
public String findUser2(@RequestParam("user") String name, Model model) {
User user;
try {
FindUserMapper rowMapper = new FindUserMapper();
user = jdbcTemplate.queryForObject(FIND_USER_BY_NAME, rowMapper, name);
} catch (EmptyResultDataAccessException e) {
user = User.NULL_USER;
}
model.addAttribute("user", user);
return "user";
}
private class FindUserMapper implements RowMapper<User>, Serializable {
private static final long serialVersionUID = 1L;
@Override
public User mapRow(ResultSet rs, int rowNum) throws SQLException {
User user = new User(rs.getLong("id"), //
rs.getString("name"), //
rs.getString("email"), //
rs.getDate("createdDate"));
return user;
}
}
}@RunWith(SpringJUnit4ClassRunner.class)
@WebAppConfiguration
@ContextConfiguration({ "file:src/main/webapp/WEB-INF/spring/appServlet/servlet-context.xml",
"file:src/test/resources/test-datasource.xml" })
public class UserAccessorIntTest {
@Autowired
private WebApplicationContext wac;
private MockMvc mockMvc;
/**
* @throws java.lang.Exception
*/
@Before
public void setUp() throws Exception {
mockMvc = MockMvcBuilders.webAppContextSetup(wac).build();
}
@Test
public void testFindUser_happy_flow() throws Exception {
ResultActions resultActions = mockMvc.perform(get("/find2").accept(MediaType.ALL).param("user", "Tom"));
resultActions.andExpect(status().isOk());
resultActions.andExpect(view().name("user"));
resultActions.andExpect(model().attributeExists("user"));
resultActions.andDo(print());
MvcResult result = resultActions.andReturn();
ModelAndView modelAndView = result.getModelAndView();
Map<String, Object> model = modelAndView.getModel();
User user = (User) model.get("user");
assertEquals("Tom", user.getName());
assertEquals("[email protected]", user.getEmail());
}
@Test
public void testFindUser_empty_user() throws Exception {
ResultActions resultActions = mockMvc.perform(get("/find2").accept(MediaType.ALL).param("user", ""));
resultActions.andExpect(status().isOk());
resultActions.andExpect(view().name("user"));
resultActions.andExpect(model().attributeExists("user"));
resultActions.andExpect(model().attribute("user", User.NULL_USER));
resultActions.andDo(print());
}
}The solution above cuts the number of first level classes to two: an implementation class and a test class. All test scenarios are catered for in a very few end to end integration tests. These tests will access the database, but is that so bad in this case? If each trip to the DB takes around 20ms or less then they'll still complete within a fraction of a second; that should be fast enough.
In terms of enhancing or maintaining this code, one small single class is easier to learn than several even smaller classes. If you did have to add in a whole bunch of business rules or other complexity then changing this code into the 'N' layer pattern will not be difficult; however the problem is that if/when a change is necessary it may be given to an inexperienced developer who'll not be confident enough to carry out the necessary refactoring. The upshot is, and you must have seen this lots of times, that the new change could be shoehorned on top of this one class solution leading to a mess of spaghetti code.
In implementing a solution like this, you may not be very popular, because the code is unconventional. That’s one of the reasons that I think that this single class solution is something that a lot of people would see as contentious. It's this idea of a standard 'right way' and 'wrong way' of writing code, rigorously applied in every case, that has lead to this perfectly good design becoming a problem.
I guess that it’s all a matter of horses for courses; choosing the right design for the right situation. If I was implementing a complex story, then I wouldn't hesitate to split up the various responsibilities, but in the simple case it's just not worth it. I'll therefore end by asking if any one has a better solution for the Simple Story Paradox shown above, please let me know.
1 I've worked on a project once in umpteen years of programming where the underlying database was changed to meet a customer requirement. That was many years and many thousands of miles away and the code was written in C++ and Visual Basic.
The code for this blog is available on Github at https://github.com/roghughe/captaindebug/tree/master/cargo-cult
2 comments:
Hi!
The problem I see, is that there is no good solution for all problems. There are only not-so-bad solutions, from which we can choose as a developers.
For instance: using DAO and Service layer may seem as a overhead for simple "find", but this overhead is often acknowledged to make methods coherent across different use-cases (the ones which actually need some action is Business Service Layer).
I agree, choose right design for right solution, but also choose design which is more-or-less coherent across whole application - that way the codebase is also more understandable for new developers. So yes, UserService just delegates finding user by id, to DAO, but also contains other methods and logic is in one place.
Cheers!
Hi Roger. Totally agree with you on this one.
Any IT problem could in theory be implemented by using custom electronics, boolean gates. etc. This would create an ultra fast and efficient system, however would be incredibly difficult to make. This is why software was invented in the first place, so that lesser mortals like us programmers, can make amazing things. I'll say that again, the *only* reason software exists, is to make it *EASY* for humans! - a few lines of code, could be orchestrating millions of hardware logic gates to produce the solution required.
I don't care what is, or isn't 'Best Practise'. The ongoing pursuit to take a single class solution, which is easily understood by even a novice programmer, splitting it down into N tier, multiple classes, does not make it easier. A maintenance programmer now has to understand the interactions between those classes in order to make changes. Its more complex. Ultimately, it will end up a even worse mess of spaghetti code, should a maintenance programmer not fully understand the original design intent and start hacking in code where it shouldn't go.
Post a comment