The other day I came across a particularly abusive post about TDD. Here’s a quote:
If you don’t use TDD in your project you are either lazy or you simply don’t know how TDD works. Excuses about lack of time don’t apply here.
I’ve been seeing this type of attitude for a while now but this one had some code attached so I decided to rant about it. First of all tests have costs, maybe the trade-off is worth it but it’s important to actually acknowledge that it’s happening in the first place.
-
Spending your limited time structuring your workflow around tests could be better spent around architecture and design of the overall code base.
-
In a poorly architected code base test give a false sense of security.
-
Most interesting use cases that you actually want to test are not amenable to testing.
-
Tests mean you have extra code that you need to maintain.
-
Writing tests manually is not particularly efficient. The purpose of good design is to decrease the surface area of possible mistakes by construction of the architecture. What does this mean in practice?
Here’s the original class from the post:
public class Account {
private String id = RandomStringUtils.randomAlphanumeric(6);
private boolean status;
private String zone;
private BigDecimal amount;
public Account() {
status = true;
zone = Zone.ZONE_1.name();
amount = createBigDecimal(0.00);
}
public Account(boolean status, Zone zone, double amount) {
this.status = status;
this.zone = zone.name();
this.amount = createBigDecimal(amount);
}
public enum Zone {
ZONE_1, ZONE_2, ZONE_3
}
public static BigDecimal createBigDecimal(double total) {
return new BigDecimal(total).setScale(2, BigDecimal.ROUND_HALF_UP);
}
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("id: ").append(getId())
.append("\nstatus: ")
.append(getStatus())
.append("\nzone: ")
.append(getZone())
.append("\namount: ")
.append(getAmount());
return sb.toString();
}
public String getId() {
return id;
}
public boolean getStatus() {
return status;
}
public void setStatus(boolean status) {
this.status = status;
}
public String getZone() {
return zone;
}
public void setZone(String zone) {
this.zone = zone;
}
public BigDecimal getAmount() {
return amount;
}
public void setAmount(BigDecimal amount) {
if (amount.signum() < 0)
throw new IllegalArgumentException("The amount does not accept negative values");
this.amount = amount;
}
}
1 & 2 & 3) Architecture/Design
There’s quite a few things here right away that I think could be improved. First this class is mutable. Someone coming from a functional language or having read “Effective Java” or “Java Concurrency in Practice” will pick up on this right away. Immutability is often a design choice. Do you want to write tests that try to verify the behavior of this class in a concurrent environment? Do you really want to accept a double precision float for money? An arguably better approach is to structure this class so that these problems don’t occur in the first place. Here’s a rewritten version:
public final class Account {
private final String id;
private final boolean status;
private final Zone zone;
private final BigRational amount;
public static Account newAccount(String id, boolean status, Zone zone, BigRational amount) {
if(id == null) throw new IllegalArgumentException("id cannot be null");
if(zone == null) throw new IllegalArgumentException("zone cannot be null");
if(amount == null) throw new IllegalArgumentException("amount cannot be null");
if(amount.isNegative()) throw new IllegalArgumentException("amount cannot be less than zero");
return new Account(id,status,zone,amount);
}
private Account(String id, boolean status, Zone zone, BigRational amount) {
this.id = id;
this.status = status;
this.zone = zone;
this.amount = amount;
}
public enum Zone {
ZONE_1, ZONE_2, ZONE_3
}
public String getId() {
return id;
}
public boolean getStatus() {
return status;
}
public Account withStatus(boolean status) {
return new Account(this.id,status,this.zone,this.amount);
}
public Zone getZone() {
return zone;
}
public Account withZone(Zone zone) {
return new Account(this.id,status,this.zone,this.amount);
}
public BigRational getAmount() {
return amount;
}
public Account addAmount(BigRational addAmount) {
if(amount == null) {
throw new IllegalArgumentException("amount cannot be null");
}
if (addAmount.isNegative()) {
throw new IllegalArgumentException("amount does not accept negative values");
}
return new Account(this.id,this.status,this.zone,this.amount.plus(addAmount));
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Account account = (Account) o;
if (status != account.status) return false;
if (!id.equals(account.id)) return false;
if (zone != account.zone) return false;
return amount.equals(account.amount);
}
@Override
public int hashCode() {
int result = id.hashCode();
result = 31 * result + (status ? 1 : 0);
result = 31 * result + zone.hashCode();
result = 31 * result + amount.hashCode();
return result;
}
private String repr;
@Override
public String toString() {
if(repr == null) {
repr = new StringBuilder()
.append("id: ").append(getId())
.append("\nstatus: ") .append(getStatus())
.append("\nzone: ") .append(getZone())
.append("\namount: ") .append(getAmount())
.toString();
}
return repr;
}
}
Is immutability harder in a language like Java than Clojure or Scala or Kotlin? Yes, but it sure saves a lot of work in the amount tests you have to write. I don’t have to worry about this class not doing the right thing with money because of weird rounding issues/floats. If you want to learn about floating point arithmetic go for it. I prefer to use Joda Money or in this case for the sake of an example I am using BigRational. You don’t have to worry about someone else extending this class and breaking encapsulation. No need to test if synchronization is done correctly. All of these things are extremely hard to unit test to the point where almost nobody does it. However if you think about the architecture of your code before writing tests perhaps you can avoid writing these tests in the first place. I’d argue the easiest things to test are quite simple and don’t end up saving much time. The real hard bugs are in the later category and it’s best to get rid of them via good architecture that doesn’t allow them to exist vs dozens of unit tests that will try to achieve the same thing.
-
Unit tests are things you have to maintain. I think most people with large code bases have experienced this. I have a 2 line change in the code that causes 20 tests to be changed. It becomes very difficult to change the architecture in a large codebase because of all the tests but the tests don’t necessarily improve or imply good architecture and design. It could be argued that tests failing when code is changed is a good thing but it’s worth acknowledging the costs associated with that kind of a workflow. In the above example I hope it’s clear that even in a simple “POJO” class where there isn’t much going on design is important.
-
It’s difficult to test interactions via unit tests, even in simple cases. Wouldn’t it be better to have your code come up with hundreds or even thousands of tests for you? With the invention of quickcheck and the porting of it to most mainstream languages it’s now possible. This can drastically decrease the amount of unit tests you have to write in the first place and I find it more convenient to write those type of tests after the code is designed and written rather than writing tests first before I have a design.
The worst part for me is that the author is probably aware of all of this since he writes Scala. The above is nothing more than a verbose version of a Scala’s case class or Kotlin’s data class.