wordy: add to track #192

stkent · 2016-11-29T03:22:25Z

Canonical data: https://github.com/exercism/x-common/blob/master/exercises/wordy/canonical-data.json

jtigger · 2016-11-29T16:03:48Z

BTW, as a reviewer, I really dig the reference to the canonical data. When reviewing, I grab the description, that data and hit the test suite, first off. Having the link in the PR is a friendly optimization.

Thanks, man!

stkent · 2016-12-03T03:25:15Z

exercises/wordy/build.gradle

+test {
+  testLogging {
+    exceptionFormat = 'full'
+    events = ["passed", "failed", "skipped"]


I added showStandardStreams = true here while I was testing my implementation; could be something we consider as standard in the future to help folks?

I totally get this. The more TDD "purist"s of my colleagues would say that if you feel the impulse to render intermediate values, redirect that energy into a unit test.

Seems like at this point in the curriculum (clearly a later-stage exercise?) we could include a HINT.md that addresses this potential need and presents both options...?

Sounds good, will keep it in mind!

stkent · 2016-12-03T03:29:35Z

exercises/wordy/src/example/java/WordProblemSolver.java

+    private static final String BINARY_OPERATION_REGEX_STRING
+        = "(plus|minus|multiplied by|divided by|raised to the power)";
+
+    private static final String VALID_QUESTION_REGEX_STRING =


Splitting into components allows for the entire problem to be validated, as well as the individual pieces of information to be extracted for the actual calculations. For example, not checking the overall structure could allow a question like

What is 3 nonsense words lorem ipsum plus 4?

to be considered valid. Still feels quite awkward though.

I initially tried writing a single regex string that would grab every piece of information required at once while simultaneously validating the entire problem structure, but then I ran into difficulties with backtracking.

insert obligatory line about regexes... like if you've got a problem then try to solve it with a regex, now you got two problems. :)

It's great mental push-ups, but likely too dense to be maintainable.

stkent · 2016-12-03T03:29:51Z

exercises/wordy/src/example/java/WordProblemSolver.java

+
+    int solve(final String wordProblem) {
+        if (!wordProblem.matches(VALID_QUESTION_REGEX_STRING)) {
+            throw new IllegalArgumentException("I'm sorry, I don't understand the question!");


Why the long face?

Our word problem solver is only so smart!

ah! yes. :)

stkent · 2016-12-03T03:30:18Z

exercises/wordy/src/example/java/WordProblemSolver.java

+
+        final Matcher initialValueMatcher = Pattern.compile(INTEGER_REGEX_STRING).matcher(wordProblem);
+
+        initialValueMatcher.find();


Guaranteed to find something, because we know the question has the appropriate structure.

Such a great example of non-defensive programming (which increases readability).

stkent · 2016-12-03T03:30:59Z

exercises/wordy/src/example/java/WordProblemSolver.java

+        int result = Integer.parseInt(initialValueMatcher.group());
+
+        final Matcher operationAndValueMatcher
+                = Pattern.compile(BINARY_OPERATION_REGEX_STRING + " " + INTEGER_REGEX_STRING).matcher(wordProblem);


Finds strings like "divided by 5" and captures the groups "divided by" and "5".

You have fun with this one! :) So delighted you're adding this exercise to this track, @stkent !

Haha yeah, this was a good exercise to build!

stkent · 2016-12-03T03:32:27Z

exercises/wordy/src/example/java/WordProblemSolver.java

+                return firstValue * secondValue;
+            case "divided by":
+                return firstValue / secondValue;
+            case "raised to the power":


Exponentiation was noted as extra credit in the common tests due to needing a different sentence structure, but it is also quite common to say "2 raised to the power 5", so I threw it in as a regular test in that format instead. Let me know if this deviation is dangerous; if so, I'll probably pull powers entirely.

I believe we should have a complete solution in the example code. So, keep it, please.

I'm not sure I clearly described what I had hoped to here. Let me try again:

In the canonical tests, exponentiation is included as extra credit based on the fact that supporting a sentence structure like "What is 52 cubed?" would require additional sentence structure matching. However, since I noted that "What is 52 raised to the power 3?" fit the existing structure, I wrote a test for that case which doesn't require new matching code. This felt opportunistic but is a deviation from the canonical data and its intent.

@jtigger does that perturb you more? I think it should.

I'll blame the cold. 😬 I now understand your original question.

It's not obvious to me...

it's a bonus iteration, so this would indicate to me a requirement that likely causes the practitioner to rethink their whole solution.

In general, I wish we had something like this for all exercises to both encourage exploration and increase fluency in the skill of deferring design choices that can be deferred (i.e. make the solution as concrete as feasible and generalize as requirements arrive).

even better is a pair of conflicting bonuses

the practitioner is being asked to write a parser, if the requirement encourages them to "mature" that part of the code, seems on target.

however, it's kindof a one-off... while that is a nice simulation of real-world software, it's a bit frustrating to the Exercism practitioner.

In the sum, I personally come down on the side of supporting the additional sentence structure.

That said, it is truly up to you: if you dig writing the more general solution, go for it. If you either disagree with the sum or you agree but want to get this off your plate, we can take this for now and amend the exercise later (if someone wants to). There's no additional cost to deferring.

@jtigger thanks for the thoughts! Agreed that this one is far from clear-cut.

I'm in the "want to get this off my 🍽" camp at this point in time, so I've simplified the implementation and replaced the old exponentiation test with a new one. PR should be good for another look!

stkent · 2016-12-03T03:33:03Z

exercises/wordy/src/example/java/WordProblemSolver.java

+            case "raised to the power":
+                return (int) Math.pow(firstValue, secondValue);
+            default:
+                throw new IllegalArgumentException("This branch should never be executed.");


Again, this should be the case because we passed the overall structure check already.

stkent · 2016-12-03T03:33:22Z

exercises/wordy/src/test/java/WordProblemSolverTest.java

+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage("I'm sorry, I don't understand the question!");
+
+        // See https://en.wikipedia.org/wiki/President_of_the_United_States if you really need to know!


Distracting, or fun?

stkent · 2016-12-03T03:33:44Z

Updated with implementation, and ready for review!

jtigger · 2016-12-06T18:48:51Z

I've been out of commission with a cold for a number of days. All I had the energy for was tweeting. ;) After a nap, here, I'll give ya a proper review, @stkent; sorry for the delay.

stkent · 2016-12-06T19:00:19Z

No rush; feel better first!

jtigger

Great stuff, @stkent !

jtigger · 2016-12-07T16:42:06Z

exercises/wordy/build.gradle

+test {
+  testLogging {
+    exceptionFormat = 'full'
+    events = ["passed", "failed", "skipped"]


I totally get this. The more TDD "purist"s of my colleagues would say that if you feel the impulse to render intermediate values, redirect that energy into a unit test.

Seems like at this point in the curriculum (clearly a later-stage exercise?) we could include a HINT.md that addresses this potential need and presents both options...?

jtigger · 2016-12-07T17:05:43Z

exercises/wordy/src/example/java/WordProblemSolver.java

+    private static final String BINARY_OPERATION_REGEX_STRING
+        = "(plus|minus|multiplied by|divided by|raised to the power)";
+
+    private static final String VALID_QUESTION_REGEX_STRING =


insert obligatory line about regexes... like if you've got a problem then try to solve it with a regex, now you got two problems. :)

It's great mental push-ups, but likely too dense to be maintainable.

jtigger · 2016-12-07T17:06:06Z

exercises/wordy/src/example/java/WordProblemSolver.java

+
+    int solve(final String wordProblem) {
+        if (!wordProblem.matches(VALID_QUESTION_REGEX_STRING)) {
+            throw new IllegalArgumentException("I'm sorry, I don't understand the question!");


Why the long face?

jtigger · 2016-12-07T17:20:09Z

exercises/wordy/src/example/java/WordProblemSolver.java

+
+        final Matcher initialValueMatcher = Pattern.compile(INTEGER_REGEX_STRING).matcher(wordProblem);
+
+        initialValueMatcher.find();


Such a great example of non-defensive programming (which increases readability).

jtigger · 2016-12-07T17:22:26Z

exercises/wordy/src/example/java/WordProblemSolver.java

+        int result = Integer.parseInt(initialValueMatcher.group());
+
+        final Matcher operationAndValueMatcher
+                = Pattern.compile(BINARY_OPERATION_REGEX_STRING + " " + INTEGER_REGEX_STRING).matcher(wordProblem);


You have fun with this one! :) So delighted you're adding this exercise to this track, @stkent !

jtigger · 2016-12-07T17:23:45Z

exercises/wordy/src/example/java/WordProblemSolver.java

+                return firstValue * secondValue;
+            case "divided by":
+                return firstValue / secondValue;
+            case "raised to the power":


I believe we should have a complete solution in the example code. So, keep it, please.

jtigger · 2016-12-07T17:27:31Z

exercises/wordy/src/test/java/WordProblemSolverTest.java

+    @Ignore
+    @Test
+    public void testExponentiation() {
+        assertEquals(32, new WordProblemSolver().solve("What is 2 raised to the power 5?"));


What do you think of splitting this out into a separate test class whose name clearly indicates it's bonus tests that can remain ignored?

Also, I'm wondering if we'd like to expand a little on the canonical data set and include more cases for this. This is inbounds, AFAIK... it's okay to provide additional cases as long as it is sufficiently additive to the experience. I think the expectation is that we contribute back to x-common.

In general, wordy seems to need some love. I've already submitted a PR to make the x-common descriptive bits... want to address some the weaknesses in the canonical data as well.

jtigger · 2016-12-07T17:27:41Z

exercises/wordy/src/test/java/WordProblemSolverTest.java

+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage("I'm sorry, I don't understand the question!");
+
+        // See https://en.wikipedia.org/wiki/President_of_the_United_States if you really need to know!


jtigger · 2016-12-07T17:29:15Z

There was a question about maybe adding a few bits in HINT.md... @stkent can you please indicate whether you want this in the PR or no? Ready to merge.

stkent · 2016-12-10T21:06:56Z

@jtigger replied! One comment requires further discussion before merge. The HINT.md adds can be added separately after #142, I think.

jtigger · 2016-12-12T16:26:40Z

@stkent back to ya.

jtigger · 2016-12-17T16:21:27Z

So, I attempted to use GitHub's new "resolve conflict" feature and apparently failed. :) Looking into it.

jtigger · 2016-12-17T16:38:13Z

Thank you, @stkent !!!

stkent changed the title ~~dibs: I will implement wordy~~ wordy: add to track Dec 3, 2016

stkent commented Dec 3, 2016

View reviewed changes

jtigger self-assigned this Dec 6, 2016

jtigger approved these changes Dec 7, 2016

View reviewed changes

stkent and others added 2 commits December 13, 2016 16:46

wordy: add to track

a57130d

Merge branch 'master' into wordy

fcb0dbc

Fixed typo in config.json from merge.

41be89b

jtigger merged commit 470f4b2 into exercism:master Dec 17, 2016

stkent deleted the wordy branch December 17, 2016 16:57


		final Matcher initialValueMatcher = Pattern.compile(INTEGER_REGEX_STRING).matcher(wordProblem);

		initialValueMatcher.find();

wordy: add to track #192

wordy: add to track #192

Conversation

stkent commented Nov 29, 2016

jtigger commented Nov 29, 2016

stkent Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stkent Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stkent commented Dec 3, 2016

jtigger commented Dec 6, 2016

stkent commented Dec 6, 2016

jtigger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtigger commented Dec 7, 2016

stkent commented Dec 10, 2016

jtigger commented Dec 12, 2016

jtigger commented Dec 17, 2016

jtigger commented Dec 17, 2016

stkent Dec 3, 2016 •

edited

Loading

stkent Dec 3, 2016 •

edited

Loading