When test driven design makes things worse

Raphael Kimmig, 30 Jul 2013

Today I watched an in­tro­duc­tion to test driv­en design us­ing an­gu­lar.js' test­ing tools.

In gen­er­al it is a nice in­tro­duct­ory talk, however one of the ex­amples used got me think­ing. The scen­ario (about 33 minutes in­to the video) is a no­ti­fi­er class that batches no­ti­fic­a­tions and sends them to the user when a cer­tain lim­it is hit.

Notifier.send = function (user, message) {
     var email = user.getDetails().email
     // enqueue email + message for eventual delivery by the backend
     ...
}

Now it is reasoned that the no­ti­fi­er hav­ing to know about the user's in­tern­al struc­ture is a bad thing (mak­ing a ref­er­ence to the law of De­meter). This leads to the fol­low­ing re­fact­or­ing at about 33:45.

Notifier.send = function (email, message) {
     // enqueue email + message for eventual delivery by the backend
     ...
}

I'd guess the real mo­tiv­a­tion for chan­ging this is the fact that when test­ing the send meth­od you need to stub out a user ob­ject to­geth­er with the de­tails. This makes the test look worse and thus cre­ates an in­cent­ive to find an­oth­er solu­tion.

Don't get me wrong, the sig­na­ture change def­in­itely has an ad­vant­age. The no­ti­fi­er doesn't need to know about the user at all - but there are some draw­backs to this:

  1. The no­ti­fi­er will nev­er be able to send no­ti­fic­a­tions via any­thing but email (which would be a per­fectly val­id design de­cision, if con­sciously made).
  2. The no­ti­fi­er is un­able to use user-spe­cif­ic pref­er­ences like pre­ferred send­ing times or per user batch set­tings.
  3. Call­ing code needs to know about the user's email ad­dress.

I think 2. would be a pretty com­mon ad­di­tion to the No­ti­fi­er. Giv­en that the No­ti­fi­er doesn't know the user we would now need to pass in the user's cus­tom batch size to­geth­er with the ad­dress. This makes the is­sue raised in 3. even more ob­vi­ous. In my opin­ion code that wants to no­ti­fy the user of some event in the ap­plic­a­tion (like "your en­cod­ing job has com­pleted") does have less of a reas­on, not more, to know the user's im­ple­ment­a­tion de­tails. Hav­ing to change every in­voc­a­tion from

send(user.getDetails().email, "your encoding job…")

to

send(user.getDetails().email, user.getDetails().batchSize, "your encoding job…")

is def­in­itely worse than just up­dat­ing the im­ple­ment­a­tion of Notifier.send.

I think this is an im­port­ant point to think about, be­cause in gen­er­al when mak­ing code more test­able you al­ways walk away with the feel­ing of hav­ing im­proved the code. And on some level you might - you just have to keep in mind that test­abil­ity is not the only met­ric of qual­ity.