Bug in JUnit MoneyBag.java example?

Discussion in 'Java' started by Jamie Andrews; real address @ bottom of message, Jun 29, 2005.

  1. 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.)
     
    Jamie Andrews; real address @ bottom of message, Jun 29, 2005
    #1
    1. Advertising

  2. Jamie Andrews; real address @ bottom of message

    Guest

    , Jun 29, 2005
    #2
    1. Advertising

  3. In comp.software.extreme-programming wrote:
    > 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.)
     
    Jamie Andrews; real address @ bottom of message, Jun 29, 2005
    #3
  4. Jamie Andrews; real address @ bottom of message

    Guest

    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);
    }
     
    , Jun 30, 2005
    #4
  5. In comp.software.extreme-programming wrote:
    > 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.


    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.)
     
    Jamie Andrews; real address @ bottom of message, Jun 30, 2005
    #5
  6. Jamie Andrews; real address @ bottom of message

    Guest

    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.
     
    , Jul 1, 2005
    #6
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Tom Koenning
    Replies:
    0
    Views:
    1,461
    Tom Koenning
    Jul 28, 2003
  2. Replies:
    1
    Views:
    5,143
    =?ISO-8859-1?Q?Arne_Vajh=F8j?=
    Nov 12, 2007
  3. Tobi
    Replies:
    0
    Views:
    1,504
  4. Ronny Mandal

    Java - Junit distinguish exceptions

    Ronny Mandal, Dec 10, 2010, in forum: Java
    Replies:
    10
    Views:
    661
    Tom Anderson
    Dec 11, 2010
  5. Sam Roberts
    Replies:
    15
    Views:
    308
    Sam Roberts
    Feb 7, 2005
Loading...

Share This Page