queen-attack: add to track #171

stkent · 2016-11-10T23:14:01Z

Test data sourced from https://github.com/exercism/x-common/blob/master/exercises/queen-attack/canonical-data.json

stkent · 2016-11-11T02:57:39Z

@jtigger no idea what caused that failure. Any clues? If not, how can I trigger a rebuild? Thanks!

stkent · 2016-11-11T03:00:57Z

Never mind, rebuild seems to be working just fine!

jtigger · 2016-11-11T13:24:12Z

hmmmmmm. From the logs:

$ bin/unit-tests.sh
FAILURE: Build failed with an exception.
* What went wrong:
Could not open terminal for stdout: $TERM not set

... but like you said, the next build succeeded.

stkent · 2016-11-11T13:53:57Z

Yeah, likely to be a CI hiccup I'm thinking.

jtigger

Good as-is. My comments focus on API design and bringing in domain concepts to increase engagement.

jtigger · 2016-11-11T14:43:25Z

exercises/queen-attack/src/example/java/QueenAttackCalculator.java

+
+    private final int[] queen2Coordinates;
+
+    public QueenAttackCalculator(final int[] queen1Coordinates, final int[] queen2Coordinates)


This definitely gets the job done. That said, I detect a hint of primitive obsession.

What do you think of codifying the concept of Coordinate?

I know that the README doesn't call it out, but what do you think of naming the parameters blackQueenCoordinates and whiteQueenCoordinates?

Personally, I find exercises that immerse me in the domain a bit more engaging... as if I'm solving a trimmed down version of "the real thing."

Just as an FYI: I'm proposing to the group to switch to SAN: exercism/problem-specifications#446. That is a significant change and should not hold up this work.

R.E. a Coordinate type - I agree that primitive types are a little ugly here, but I avoided introducing a second separate type that requires implementation as I did not find another example which follows that pattern. (This is probably why other exercises used e.g. Guava to provide a nicer parameter type, though I agree with the removal of that third party dependency moving forward). It shouldn't be hard to adapt the problem, however, so I'll make the change to use a separate Coordinate type and we can see how that feels!

R.E. renaming to white/black - yep, great call, will make the change!

R.E. notation change - there's a note about using chess notation in the canonical data set that mentions it, but IMO it's not really clear how the note should be interpreted:

Some languages implement tests beyond this set, such as checking for two pieces being placed on the same position, representing the board graphically, or using standard chess notation. Those tests can be offered as extra credit.

Is there build-in support in Exercism for optional/extra credit tests? If not, I'm guessing this means we could expose two methods that accept differently-formatted Coordinate types and require implementations that satisfy both to consider the problem complete. That feels like relatively uninteresting duplication, however.

subject Coordinates:
It is absolutely "in bounds" to include other bits in the starter code (i.e. main sourceset). There are examples of this in other exercises (Allergen for one).

The way I think about it, we are defining the API for the exercising programmer. We are free to define as little as an interface and as much as a whole class as we see appropriate.

subject "SAN":
There is not currently a mechanism for extra-credit, per se. It's merely a mention of where the exercising programmer could go from here.

Personally, I'd like to see this exercise be defined as using SAN, but that takes rough consensus across the tracks (read: not happening, today).

It could be an interesting exercise to declare two contracts (one with cartesian coordinate and another for SAN coordinates) and a test class for both of those interfaces. The SAN test class would have to make it clear that it is optional; there ought to be additional readme (EXTRA-CREDIT.md?) to provide a proper framing.

None of that is at all required for this PR. I'm just thinking out loud, now.

R.E. Coordinates: ah, thanks for the reference to the allergies exercise; symlinking into the example source set is a nice solution there. What are your thoughts, then, for the queen-attack exercise: require the implementer to build the Coordinate class, or provide it and allow the implementer to focus on the attack logic only?

R.E. notation: all good points! Perhaps you could open a new issue to capture the ideas you described regarding extra credit, etc.?

jtigger · 2016-11-11T14:51:23Z

exercises/queen-attack/src/test/java/QueenAttackCalculatorTest.java

+public final class QueenAttackCalculatorTest {
+
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();


Nice. This is a lovely JUnit idiom to introduce here.

At this time, this is the first time we've used JUnit Rules. What would be the "right" user experience here?

just discover it and have her/him do her/his own research to discover how this mechanism works?

see a comment that's a URL to an introduction to JUnit Rules?

see a comment that references a markdown file in the fetched exercise?

I like the second option - we can save a search and provide a link to: https://github.com/junit-team/junit4/wiki/Rules, but still leave reading and discovery to the user?

jtigger · 2016-11-11T14:53:04Z

exercises/settings.gradle

@@ -24,6 +24,7 @@ include 'pangram'
 include 'pascals-triangle'
 include 'phone-number'
 include 'pig-latin'
+include 'queen-attack'


I really appreciate the little ways you keep things tidy, @stkent. This is a small thing, but instead of just adding this to the end of the list, you included it alphabetically👍 . You're probably just enabling my OCD. 😉

Hah, everything in its right place!

jtigger

The actual "request changes" part is the seemingly extraneous Coordinate.

I can't help myself, so I also included some other suggestions; feel free to ignore, @stkent — this is really lovely, as is.

jtigger · 2016-11-14T03:46:59Z

exercises/queen-attack/src/main/java/Coordinate.java

@@ -0,0 +1,5 @@
+public final class Coordinate {


I suspect this was left over from an intermediate effort?

Ah, yes, I did mean to leave a BoardCoordinate class in here since that's a required part of the exercise API; renamed!

jtigger · 2016-11-14T03:53:07Z

exercises/queen-attack/src/example/java/BoardCoordinate.java

+
+    private final int file;
+
+    public BoardCoordinate(final int rank, final int file) throws IllegalArgumentException {


This is not at all a request/demand... just a thought: value objects often come with come with a factory method (usually with a short name). Then, in places where it is used, they employ a static import.

public static BoardCoordinate at(final int rank, final int file) { return new BoardCoordinate(rank, file); }

While I'm familiar with the practice, in this particular code I felt that it would hurt readability. For example, I think that the context provided by the class name here:

new QueenAttackCalculator(new BoardCoordinate(2, 4), new BoardCoordinate(6, 6));

makes that code easier to comprehend than, say:

new QueenAttackCalculator(at(2, 4), at(6, 6));

This is especially true given that users must attempt to reverse-engineer the APIs required in their implementations from reading the tests (static imports of non-test-framework classes definitely make this harder!)

oooohhhh! I missed the part where BoardCoordinates is in the example sourceset. I absolutely agree.

jtigger · 2016-11-14T03:55:47Z

exercises/queen-attack/src/test/java/QueenAttackCalculatorTest.java

 import static org.junit.Assert.assertTrue;

 public final class QueenAttackCalculatorTest {

+    /*


stkent · 2016-11-14T13:57:03Z

@jtigger thanks for the comments - replied/updated!

jtigger · 2016-11-14T16:25:33Z

exercises/anagram/src/test/java/AnagramTest.java

-import org.junit.Test;
-import org.junit.Ignore;
+
+import static org.assertj.core.api.Assertions.assertThat;


OMG, you're going to kill me — this is a long PR; thank you for your patience.

Looks like we have a freeloader, here.

The pitfalls of auto-optimized imports! Fixed this commit :)

stkent · 2016-11-14T19:18:06Z

Rebased to fix conflicts after largest-series-product was merged.

jtigger · 2016-11-14T20:09:49Z

Thanks again, @stkent. Good add.

stkent · 2016-11-19T02:09:15Z

@jtigger I don't see this exercise on the website (http://exercism.io/languages/java/exercises) 4 days after the merge. Is there a periodic rebuild of the website that will expose the new exercise, or did I miss something in implementing this PR? Thanks!

jtigger · 2016-11-19T03:50:07Z

You have done your job, @stkent.

Your work typically gets swept-up in the periodic deploys that @kytrinyx does from time-to-time. With that mention, she's now explicitly aware of this one. ;)

stkent · 2016-11-19T14:32:30Z

Hah, as I suspected. Thanks for confirming!

stkent changed the title ~~dibs: I will implement exercise queen-attack~~ queen-attack: add to track Nov 11, 2016

jtigger suggested changes Nov 11, 2016

View reviewed changes

jtigger assigned stkent Nov 12, 2016

stkent mentioned this pull request Nov 12, 2016

largest-series-product: add to track #173

Merged

jtigger suggested changes Nov 14, 2016

View reviewed changes

stkent added 6 commits November 14, 2016 14:17

queen-attack: add to track

cda0e7b

Refactor queen-attack to use a Coordinate type

4f4dbc8

Add missing canonical test case to queen-attack

906240b

Add context r.e. JUnit Rules to queen-attack tests

dded811

Resequence/rename queen-attack tests

21b8ca2

Rename queen-attack starter class

4ed70af

jtigger merged commit 5f83bc9 into exercism:master Nov 14, 2016

stkent deleted the queen-attack branch November 14, 2016 20:13

stkent mentioned this pull request Nov 26, 2016

robot-simulator: add to track #184

Merged


		private final int[] queen2Coordinates;

		public QueenAttackCalculator(final int[] queen1Coordinates, final int[] queen2Coordinates)


		private final int file;

		public BoardCoordinate(final int rank, final int file) throws IllegalArgumentException {

Uh oh!

queen-attack: add to track #171

queen-attack: add to track #171

Uh oh!

Conversation

stkent commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stkent commented Nov 11, 2016

Uh oh!

stkent commented Nov 11, 2016

Uh oh!

jtigger commented Nov 11, 2016

Uh oh!

stkent commented Nov 11, 2016

Uh oh!

jtigger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtigger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stkent commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stkent commented Nov 14, 2016

Uh oh!

jtigger commented Nov 14, 2016

Uh oh!

stkent commented Nov 19, 2016

Uh oh!

jtigger commented Nov 19, 2016

Uh oh!

stkent commented Nov 19, 2016

Uh oh!

Uh oh!

stkent commented Nov 10, 2016 •

edited

Loading