Bug in JUnit MoneyBag.java example?

[Jamie Andrews; real address @ bottom of message]

Hi... Is there supposed to be a bug in the MoneyBag.java
code, from the "money" example that comes standard with JUnit?

--Jamie. (Celebrating (?) 20 years on Usenet!)
andrews .uwo } Merge these two lines to obtain my e-mail address.
@csd .ca } (Unsolicited "bulk" e-mail costs everyone.)

 

[jeffgrigg]

Do you think you found a bug?

Do you have a test that demonstrates it?

If so, you might get more interest in the JUnit discussion group:
http://groups.yahoo.com/group/junit/

 

[Jamie Andrews; real address @ bottom of message]

Do you think you found a bug?
Do you have a test that demonstrates it?

Yes, but it might be just a misunderstanding about what
appendTo() and equals() are supposed to do. Make a MoneyBag
with 5 CHF and 5 USD. Make a Money with -5 CHF. Append the
latter to the former with a call to appendTo(). If I assume the
correct intended specification for appendTo(), we should get
something with just 5 USD. However, the result is not equal to
a Money with just 5 USD in it. Here's the code.

public class MoneyBug {

public static void main(String[] args) {
MoneyBag moneyBagTarget = (MoneyBag)
MoneyBag.create(new Money(5, "CHF"), new Money(5, "USD"));
Money minusFiveFrancs = new Money(-5, "CHF");
Money fiveBucks = new Money(5, "USD");
minusFiveFrancs.appendTo(moneyBagTarget);

if (moneyBagTarget.equals(fiveBucks)) {
System.out.println("No bug");
} else { // Shouldn't they be equal?
System.out.println("Bug");
System.out.println("moneyBagTarget: " + moneyBagTarget);
System.out.println("fiveBucks: " + fiveBucks);
}
}
}

The problem is that the MoneyBag now contains just one
currency, but MoneyBag.equals() essentially assumes that a
MoneyBag has more than one currency (it returns false if it is
being compared to a Money). Note that toString() on the
MoneyBag returns "{[5 USD]}".

Interestingly Kent Beck in _TDD By Example_ addresses a
very similar problem in his discussion of the add() method and
equals(). It is fixed there. Almost every operation ultimately
comes down to a MoneyBag.create() call, and create() calls
simplify() is called in order to make create() return a Money if
there is only one currency left. So MoneyBag.equals() assumes
that "this" has more than one currency and cannot be equal to a
Money. However, appendTo() does not always create such a MoneyBag.

appendTo() has to be an IMoney method because of the way it
is called in MoneyBag. But what is the specification? If it is
essentially supposed to add the method receiver to the method
parameter, then it is producing something of a form not expected
by equals(). This could be classified as a bug in appendTo() or
a bug in equals(). I think the easiest way to fix it is to
change equals() so that the MoneyBag "{[5 USD]}" is equal to the
Money "[5 USD]". Of course I may just have misunderstood what
appendTo() and/or equals() are supposed to do.

This bug (if it is a bug) was found using a new tool,
RUTE-J (Randomized Unit Testing Engine for Java), programmed by
my research group.

If so, you might get more interest in the JUnit discussion group:
http://groups.yahoo.com/group/junit/

I'll take a look there. Thanks!

--Jamie. (Celebrating (?) 20 years on Usenet!)
andrews .uwo } Merge these two lines to obtain my e-mail address.
@csd .ca } (Unsolicited "bulk" e-mail costs everyone.)

 

[jeffgrigg]

Interesting. This looks like a bug in the sample program.

(Of course, someone might claim that it was put in as an exercise to
test the student! ;-)

From taking a quick look at the code, I suspect that 'appendTo' isn't

really intended to be a part of the public interface for the Money and
MoneyBag classes. In the Money class, I see this line:
public /*this makes no sense*/ void appendTo(MoneyBag m) {

I think this comment suggests that at least one person thought that
'appendTo' shouldn't be part of the public interface. I think that
clients are expected to call the 'add' method, not the 'appendTo'
method. However, given the way that MoneyBag uses it, the method has
to be in the interface. Unfortunately, one can't declare methods
'protected' or "package protected" in an interface. ;->

Expressing your test and what I think is expected usage as tests:

public void testMoneyBagToMoneyComparison() {
MoneyBag moneyBagTarget = (MoneyBag)
MoneyBag.create(new Money(5, "CHF"), new Money(5, "USD"));
Money minusFiveFrancs = new Money(-5, "CHF");
Money fiveBucks = new Money(5, "USD");
minusFiveFrancs.appendTo(moneyBagTarget);

assertEquals(fiveBucks, moneyBagTarget); // This fails.
}
public void testMoneyBagToMoneyComparison2() {
MoneyBag moneyBagTarget = (MoneyBag)
MoneyBag.create(new Money(5, "CHF"), new Money(5, "USD"));
Money minusFiveFrancs = new Money(-5, "CHF");
Money fiveBucks = new Money(5, "USD");

IMoney fiveUSD = moneyBagTarget.add(minusFiveFrancs);
assertEquals(fiveBucks, fiveUSD);
}

 

[Jamie Andrews; real address @ bottom of message]

Interesting. This looks like a bug in the sample program.
(Of course, someone might claim that it was put in as an exercise to
test the student! ;-)
really intended to be a part of the public interface for the Money and
MoneyBag classes.

I think you're right. It doesn't make much sense from an
API design point of view, since it seems to duplicate the
functionality of add().

In the Money class, I see this line:
public /*this makes no sense*/ void appendTo(MoneyBag m) {

Yes, we weren't sure if that referred to just the
visibility or to the implementation. Probably the visibility,
though, because of where it was positioned.

I think this comment suggests that at least one person thought that
'appendTo' shouldn't be part of the public interface. I think that
clients are expected to call the 'add' method, not the 'appendTo'
method. However, given the way that MoneyBag uses it, the method has
to be in the interface. Unfortunately, one can't declare methods
'protected' or "package protected" in an interface. ;->
Expressing your test and what I think is expected usage as tests:

[code snipped] Thanks for the JUnit version.

Just as a side note, the same problem can arise when we try
to append two MoneyBags. For instance, appending the MoneyBag
{[5 CHF],[5 USD]} to the MoneyBag {[-5 CHF],[5 USD]} does not
result in a MoneyBag that is equals() to [10 USD]. So, even if
appendTo() were used just within MoneyBag, something would still
have to change.

--Jamie. (Celebrating (?) 20 years on Usenet!)
andrews .uwo } Merge these two lines to obtain my e-mail address.
@csd .ca } (Unsolicited "bulk" e-mail costs everyone.)

 

[jeffgrigg]

I implemented Money and MoneyBag in a production system recently, but
did not see any reason to make them interchangable or directly
comparable, as the JUnit example does -- a factor leading to this bug.