xp-dojo / atdd-bank-account Goto Github PK

As discussed in #3 lets remove the binaries and .idea project files and rely on gradle instead.

I still think this isn't the approach going forward - I'd prefer a zero deps thing that "just works".

I also think that maven is lower touch than gradle (built in plugin) so propose we keep both the pom.xml and gradle.build files in sync. Open to discussed this though (if we do, we really should setup travis or circeci to keep us honest).

Once the whole thing is developed and we prepare for delivery, I think we should remove the build files as previously discussed.

Not really sure about some of the annotations from J5. @DisplayName and @Nested. Personally, I think it's a bit twee and shy away from annotations generally but realise it's subjective. For the pedagogical, I don't think it adds anything and may be distracting.

It's hard to make a call because if the audience is happy with the syntax, it'll feel natural but for noobs, it'll feel scary. My default when put in this position, is to simplify.

As for OSS, should we add copyright banners to all the files?

I'm keen to keep it open for people to use in classrooms but want to assert our rights at the same time

Merging the concordion branch has raised a few questions.

I don't think the project works out of the box as is, I couldn't open in IDEA from a clean clone. Wasgit rm --cached run on the bits to remove - was stuff removed at all? I'm a bit confused on it.

Anyway, the Concordion stuff, I brought in with Gradle and Maven support 😱 and now aren't sure what to do in the interest of gettings things cooking.

I may update the POM and build.gradle (for now) so its belts and braces, but let's use this issue to track our thoughts. Spoke to Suggs the other day who is 👨🐜(adam-ant) on gradle support...

At the risk of too much WIP, I thought it would be straight forward but https://travis-ci.com/xp-dojo/atdd-bank-account/jobs/183692865 says otherwise.

Now all I can think about is getting this green.

Let's pick one (hamcrest/assertj) and make it consistent. They both seem pretty readable to me so I can't see a compelling reason to have both in there (and I don't mind which we settle on).

See https://stackoverflow.com/questions/55250429/concordion-can-not-copy-resource-all-of-a-sudden

Concordion will try and create a file which if it contains relative paths, can fail with file not found errors.

For example:

ll /var/folders/wg/3nnv2yf90914pqpx69wmd9cc0000gn/T/concordion/org/xpdojo/bank/../../../concordion.css (No such file or directory

because only concordion above exists, it hasn't gotten around to creating the org/xpdojo/bank bit yet, so will fail.

Reproducible on Mac and Linux by deleting the temp folder.

I propose we structure the test packages something like the following:

src
 ├───test
 │       ├───unit
 │       │       ├───java
 │       │       └───resources
 │       └───acceptance
 │               ├───java
 │               └───resources
 └─── ...

The point being to make it clearer for attendees what's an acceptance test and what's a "unit"-style test.

Trouble I'll have is setting up Gradle 😢

TiL: the combiner in the Stream#reduce method won't be used unless the stream is parallel.

This means the following line is technically incorrect, you can pass in nonsense for the last parameter and everything still works.

A test to show the problem is below.

@Test
void generateBalanceForALargerSetOfTransactions() {
    Account account = accountWithBalance(amountOf(10), FIXED_CLOCK);
    final int numTransactions = 1000;
    LongStream.iterate(10, operand -> operand).limit(numTransactions).forEach(value -> account.deposit(amountOf(value)));
    assertThat(account.balance(), is(amountOf(10 + 10 * numTransactions)));
}

The "contract" for reduce should ensure the following - the combiner must be "compatible" with the accumulator for all values.

combiner.apply(u, accumulator.apply(identity, t)) == accumulator.apply(u, t)

So in terms of associativity;

combiner.apply(u, t) == combiner.apply(t, u)

A test to prove it:

@Test
void checkCombinerConformsToReduceSpecification() {
    // the "spec": combiner.apply(u, accumulator.apply(identity, t)) == accumulator.apply(u, t)

    BiFunction<Money, Money, Money> invalidCombiner = (a, b) -> a;
    BiFunction<Money, Money, Money> combiner = Money::plus;
    BiFunction<Money, Transaction, Money> accumulator = (money, transaction) -> transaction.against(money);

    Money u = amountOf(100);
    Transaction t = depositOf(amountOf(200), Instant.now());
    assertThat(invalidCombiner.apply(u, accumulator.apply(ZERO, t)), is(not(accumulator.apply(u, t))));
    assertThat(combiner.apply(u, accumulator.apply(ZERO, t)), is(accumulator.apply(u, t)));
}

To get the test to pass (forcing parallel stream):

 public Money balance() {
     return transactions().parallel().reduce(ZERO, (money, transaction) -> transaction.against(money), Money::plus);
 }