robot-simulator: add to track #184

stkent · 2016-11-24T04:44:13Z

Test data from here. I omitted "negative positions are allowed" since the GridPosition implementation is provided as part of the exercise. Otherwise, all common tests should be present.

stkent · 2016-11-26T01:23:18Z

@jtigger any idea what I might be doing wrong here? Looks like the first run through of the tests on CI passed, but the second run through failed:

==================================================
46 of 46 -- robot-simulator
==================================================
::1 - - [26/Nov/2016:01:10:23 +0000] "GET /v2/exercises/java/robot-simulator HTTP/1.1" 404 66 0.0030
2016/11/26 01:10:23 unable to fetch problems (HTTP: 404) - No implementation of 'robot-simulator' in track 'java'
>>> on_exit()
stopped x-api (pid=26780)
<<< on_exit()

jtigger · 2016-11-26T06:31:52Z

Yeah, I can see at least part of it. The journey test constructs a local copy of trackler. I see a defect in that part of the test code in that it doesn't ensure that the version of trackler created in that step is the same version used in building and starting x-api.

When I tried to run the script locally, I can't even get it to connect properly to x-api... looking into it.

jtigger · 2016-11-26T08:54:36Z

Okay, my local issue was unrelated.

Since this is a issue, I've committed the fix to bin/journey-test.sh on exercism/master; I recommend you fetch+rebase. I did test this on your branch and it worked for me, locally.

I'm entirely unhappy with how hacky this first cut is; it introduces connascence of position when it should be doable with less coupling (i.e. matching exactly on the locked version number in the Gemfile.lock file — omitting the dependency entry). I ran into apparent non-portable syntax between OS X's grep and Ubuntu 12's. It's gotten late and I wanted to do something to unblock the track.

jtigger · 2016-11-26T09:12:22Z

exercises/robot-simulator/src/test/java/RobotTest.java

+        assertTrue(actualEndRobotOrientation.equals(expectedEndOrientation));
+    }
+
+    private void testInstructions(


Hmmmmmmmmmmmm.... so, I get the DRYing up of the tests. But I'm personally apprehensive about trading-off any readability from within a given test. I think this Extract Method is okay, I'll need to sleep on it myself.

What do others think? @exercism/java

@stkent @jtigger in general I like the fact that these helper methods are pulled out, but it does make me squint a little when following the tests. There's definitely a school of thought that says tests should be dead simple, even to the point of eschewing any logic to create data objects programmatically for testing. That camp would prefer to see everything spelled out literally.

I don't have a strong feeling about this either way, but we should just consider the audience. The tests contain a considerable amount of the guidance that the users get, we should make sure they're as useful as possible.

Yes, I went back and forth on this myself (at one point, I had similar helper methods extracted to test the "turn"-type logic, but ended up writing those out explicitly). For consistency and the sake of being super-explicit, I'll go ahead and remove this helper method too!

In a production test suite, I'd personally want to remove the duplication. However, in Exercism, the practitioner is digesting these tests one-at-a-time. As such, it is my belief that explicitness is particularly crucial.

What do you both think of the testTurningRight and testTurningLeft methods? I've already removed the testInstructions method locally.

Personally, I'd inline them.

To come to that conclusion I did this little exercise with myself: I pretended I was seeing this exercise for the first time and just myopically walked through the test methods from the top. When I got to testTurningRightCorrectlyChangesOrientation(), I had to then jump down to the definition of testTurningRight() which, on examination, didn't include any more "boilerplate" than other test methods... at which point the whiny pedantic voice in my head said, "why not just include that in the test above?"

Roger that; willfix!

jtigger · 2016-11-26T09:18:16Z

exercises/robot-simulator/src/main/java/GridPosition.java

+        this.y = y;
+    }
+
+    boolean equals(final GridPosition gridPosition) {


Can you talk about the decision to overload instead of override equals()? Not saying it's wrong, just want to talk it through.

Here's what a full implementation of equals and hashCode looks like:

@Override public boolean equals(final Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; final GridPosition that = (GridPosition) o; if (x != that.x) return false; return y == that.y; } @Override public int hashCode() { int result = x; result = 31 * result + y; return result; }

Since this is a class supplied as part of the starter implementation, my thinking was that the above represented a lot of irrelevant detail a user might think they needed to understand in order to proceed with the problem.

Excellent point. Agreed.

We also want to balance that with the principle that Exercism code should be exemplary.

What do you think of some succinct comment above or within equals() that include a URL to an accessible description of the rationale behind keeping equals() and hashCode() in sync, semantically?

I think your proposal is a good balance, and will take a stab at it locally with the expectation that we might iterate a couple of times to capture all that needs to be said!

Related-ish: are reference solutions ever exposed to users? I ask in light of this comment (my emphasis):

The reference solution is named something with example or Example in the path.

The solution does not need to be particularly great code, it is only used to verify that the exercise is coherent.

I think that exposing users to exemplary reference solutions should be the norm; just wondering where Exercism as a whole stands on that front?

jtigger · 2016-11-26T09:20:09Z

oh yeah! I forgot that PRs will automatically rebase. Once I re-kicked the build with the fix on exercism/master, this passed. :)

matthewmorgan · 2016-11-26T11:40:28Z

@stkent another fine effort.

I hate to be that guy, but I need to raise one concern: I'm a little hesitant about this and queen-attack requiring multiple business classes to implement the solution. I've dabbled in several of the tracks on Exercism and that seems to be a departure from the usual pattern, regardless of the language track. There are some precedents, I've seen a few enums, for example.

Are commenters able to see multiple files in the UI when reviewing someone's submission? I don't even know the answer to that question.

My thought is that going in this direction has both benefits and costs, and warrants some discussion and a decision. What do you think @stkent ? @exercism/java ?

stkent · 2016-11-26T14:32:37Z

@matthewmorgan definitely a valid concern, thanks for raising it! I will note that there is a slight difference between the cases you mention:

in queen-attack, the user must create implementations in two separate classes (BoardCoordinate and QueenAttackCalculator);
in this pull request, the user must create an implementation in just one class (Robot) that relies on a provided implementation in another class (GridCoordinate).

The latter is a pre-existing pattern used in e.g. Allergen, and was identified as being 👍 by @jtigger. You are right, though, that this is a full class (a little bit more complex than an enum; but barely, in this case).

As additional context: my first run at queen-attack kept all implementation in a single class, but was updated at @jtigger's suggestion to avoid a lot of primitive wrangling.

Requiring implementation in two separate classes (as in queen-attack) makes me a lot more uneasy than asking users to leverage supplied classes. This might be veering a little off-topic now, but the need for additional coordinate logic in queen-attack was largely driven by the tests that verify input validity. Several of the exercises I've added to date include tests that require exceptions be thrown if input is invalid, but I've noticed that (in general) the strictness with which input is validated is inconsistent across the common exercises (and in the common data). I think there are two good approaches to this inconsistency:

standardize on minimal input validation throughout the track (or throughout all tracks);
begin the track with no input validation, and ramp up the required validation as the difficulty of exercises increases.

Yet another task that may depend on #142.

Thoughts?

stkent · 2016-11-26T14:33:20Z

Regarding:

Are commenters able to see multiple files in the UI when reviewing someone's submission? I don't even know the answer to that question.

I'm not sure; it's certainly possible to submit multiple files for a problem, as I recently discovered, but I have not yet inspected such a submission from the other side.

stkent · 2016-11-26T14:33:52Z

@jtigger thanks for investigating and addressing that failure!

stkent · 2016-11-26T15:05:56Z

@matthewmorgan confirmed, reviewers can see multiple files if they are submitted by the user; however, the CLI info page needs an update to let people know that's permitted! Some submissions therefore include the QueenAttackCalculator implementation only, and no BoardCoordinate component.

jtigger · 2016-11-27T17:05:34Z

@matthewmorgan (being "that guy" 🙄)

... but I need to raise one concern: I'm a little hesitant about this and queen-attack requiring multiple business classes to implement the solution.

@stkent differentiated between exercises that are supplying multiple classes vs. requiring them.

I agree, this is an important concern:

Yes: keep things as simple as possible.
Should we shy away from having multiple classes?
- Java is an Object-Oriented language. OO's purpose, in part, is to tackle complexity by providing the ability to break logic into chunks. SRP reminds us that the smaller the chunks, the more resilient to change, and therefore more valuable — following SRP leads to getting to multiple classes sooner.
- if by the end of the track, the practitioner hasn't seen meaningful class design (of which SRP is a part) have we given them a sufficient exposure to the language?

the mechanics can be straight-forward:

supplied classes are "easy" because they are ... supplied — the practitioner just uses them.
required classes could be supplied as interfaces, allowing the practitioner to decide how they want to organize their solution (we could include a HINT.md for these exercises that suggest the simplest way: include as a non-public top-level class that implements the interface).

stkent · 2016-11-27T18:43:14Z

+1 if a multi-class OO solution fits best, we shouldn't be afraid of that!

Regarding:

required classes could be supplied as interfaces, allowing the practitioner to decide how they want to organize their solution (we could include a HINT.md for these exercises that suggest the simplest way: include as a non-public top-level class that implements the interface).

I see what you're getting at with this, @jtigger, but I wonder if the introduction of the interface here would be more confusing than instructive...

stkent · 2016-11-27T18:44:21Z

Another thought: could we include an appropriate (multi-file) CLI submit command for multi-file problems in the appropriate HINT.md files?

stkent · 2016-11-27T20:49:39Z

@jtigger @matthewmorgan updated, and ready for another look!

matthewmorgan · 2016-11-27T23:36:07Z

@jtigger @stkent I see that in this case we're supplying the classes required-- that's an oversight on my part, I apologize. I think that's just fine.

I also have heard @jtigger that sometimes people use more than one class in a Java application, although personally as a super-programmer I always just use one 😈

I guess I'm with @stkent on the thought of introducing an interface, but I could be convinced that it has real value too. Maybe we should deliberately introduce that at some point. If we did, I think a HINT.md would be a capital idea.

Anyway, as I said, nice work, I would give this PR a 👍

stkent · 2016-11-28T00:58:49Z

sometimes people use more than one class in a Java application

Heathens!

jtigger · 2016-11-28T15:30:18Z

@matthewmorgan said:

I also have heard @jtigger that sometimes people use more than one class in a Java application, although personally as a super-programmer I always just use one 😈

I've heard that about you... :)

jtigger · 2016-11-28T15:40:03Z

@stkent:

I see what you're getting at with this, @jtigger, but I wonder if the introduction of the interface here would be more confusing than instructive...

@matthewmorgan:

I guess I'm with @stkent on the thought of introducing an interface, but I could be convinced that it has real value too. Maybe we should deliberately introduce that at some point. If we did, I think a HINT.md would be a capital idea.

My "argument" is based on seeing the interface as a fundamental feature of the language. If so, then the question is not if we should introduce them, but when. One criteria would be that the exercise naturally circumscribes a sufficiently complex domain that more than one non-primitive type is appropriate.

I love the notion of making use of the HINT.md file for these kinds of things; it's automatically included into the README.md, so there's no extra work on our part to bring attention to it.

jtigger · 2016-11-28T15:48:01Z

@stkent:

... the need for additional coordinate logic in queen-attack was largely driven by the tests that verify input validity. Several of the exercises I've added to date include tests that require exceptions be thrown if input is invalid, but I've noticed that (in general) the strictness with which input is validated is inconsistent across the common exercises (and in the common data). I think there are two good approaches to this inconsistency:

standardize on minimal input validation throughout the track (or throughout all tracks);

begin the track with no input validation, and ramp up the required validation as the difficulty of exercises increases.
Yet another task that may depend on Come up with a draft ordering of the exercises in xJava #142.

Thoughts?

Yes! Validation arises as a Responsibility (invoking SRP, again); I can see how it could be the raison d'être of a Value Object.

I dig the notion of "validation" being an aspect of later exercises. But I feel they'll be worthwhile/fun when included as needed.

And yeah, #142 is just churning up all kinds of fantastic aspects! :)

jtigger

Oh yeah... there's a PR in all there isn't there?!? 😉

Personally, I'm digging the more explicit test methods; feels more approachable — thanks for doing the legwork.

A few suggestions in the review.

jtigger · 2016-11-28T15:49:20Z

exercises/robot-simulator/src/main/java/GridPosition.java

+        this.y = y;
+    }
+
+    /*


jtigger · 2016-11-28T15:49:57Z

exercises/robot-simulator/src/main/java/Robot.java

@@ -0,0 +1,5 @@
+final class Robot {


Should this class be public?

Don't think it needs to be; the tests are located in the same package, so package-private access is sufficient for the class and it's exposed methods!

I see whatcha doin' there. Cool!

jtigger · 2016-11-28T15:54:43Z

exercises/robot-simulator/src/test/java/RobotTest.java

+
+        final GridPosition actualGridPosition = robot.getGridPosition();
+
+        assertTrue(actualGridPosition.equals(initialGridPosition));


What are your thoughts on using assertEquals()? This increases the fidelity of signal from the test system: the programmer can know what's wrong just looking at the failure message.

I think assertEquals delegates to the Object-based equals implementation, so this test (and others) would fail if I used it. I can add an explicit failure message here as an alternative?

jtigger · 2016-11-28T16:00:35Z

exercises/robot-simulator/src/test/java/RobotTest.java

+
+        final Orientation actualOrientation = robot.getOrientation();
+
+        assertTrue(actualOrientation.equals(initialOrientation));


I'm wondering if actualOrientation is carrying its intention-revealing weight, here. How does this compare with:

@Test public void testRobotIsCreatedWithCorrectInitialOrientation() { final Orientation initialOrientation = Orientation.NORTH; final Robot robot = new Robot(new GridPosition(0, 0), initialOrientation); assertEquals(robot.getOrientation(), initialOrientation); }

?

Good call; will fix throughout the tests!

jtigger

Thanks again for both doing such quality work and enduring nitpicky scrutiny. Another great add to the track!

stkent changed the title ~~dibs: I will implement exercise robot-simulator~~ robot-simulator: add to track Nov 26, 2016

jtigger reviewed Nov 26, 2016

View reviewed changes

jtigger reviewed Nov 28, 2016

View reviewed changes

jtigger self-assigned this Nov 28, 2016

robot-simulator: add to track

78295f4

jtigger approved these changes Nov 29, 2016

View reviewed changes

jtigger merged commit 66d2a90 into exercism:master Nov 29, 2016

stkent deleted the robot-simulator branch November 29, 2016 14:25


		final GridPosition actualGridPosition = robot.getGridPosition();

		assertTrue(actualGridPosition.equals(initialGridPosition));


		final Orientation actualOrientation = robot.getOrientation();

		assertTrue(actualOrientation.equals(initialOrientation));

Uh oh!

robot-simulator: add to track #184

robot-simulator: add to track #184

Uh oh!

Conversation

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

Uh oh!

Uh oh!

stkent commented Nov 26, 2016

Uh oh!

jtigger commented Nov 26, 2016

Uh oh!

jtigger commented Nov 26, 2016

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!

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

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 commented Nov 26, 2016

Uh oh!

matthewmorgan commented Nov 26, 2016

Uh oh!

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

Uh oh!

Uh oh!

stkent commented Nov 26, 2016

Uh oh!

stkent commented Nov 26, 2016

Uh oh!

stkent commented Nov 26, 2016

Uh oh!

jtigger commented Nov 27, 2016

Uh oh!

stkent commented Nov 27, 2016

Uh oh!

stkent commented Nov 27, 2016

Uh oh!

stkent commented Nov 27, 2016

Uh oh!

matthewmorgan commented Nov 27, 2016

Uh oh!

stkent commented Nov 28, 2016

Uh oh!

jtigger commented Nov 28, 2016

Uh oh!

jtigger commented Nov 28, 2016

Uh oh!

jtigger commented Nov 28, 2016

Uh oh!

jtigger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

stkent commented Nov 24, 2016 •

edited

Loading

stkent Nov 26, 2016 •

edited

Loading

stkent commented Nov 26, 2016 •

edited

Loading

jtigger left a comment •

edited

Loading

stkent Nov 29, 2016 •

edited

Loading