implement say exercise by mirkoperillo · Pull Request #1769 · exercism/java

mirkoperillo · 2019-10-06T20:27:19Z

Add folder structure and settings for say exercise.
Work in progress. Fixes #1752

mirkoperillo · 2019-10-07T12:15:12Z

Hi @lemoncurry this is my proposal for say exercise.
I miss some settings, I think: how to produce README and how to add exercise entry into config.json.
Can you give me some hints about this to take the right way ? Thank you

lemoncurry · 2019-10-07T13:51:27Z

@mirkoperillo How to create the README.md is described here.
You can just add a new entry for this exercise into config.json. You may decide on the difficulty level, topics, etc.
Let me know if you have further questions.

mirkoperillo · 2019-10-08T07:51:55Z

done @lemoncurry.
waiting for the review

lemoncurry · 2019-10-09T12:27:19Z

Thank you @mirkoperillo for contributing again to the Java track 😄
There seems to be a violation of the style rules, could you please check that?
Then I will review your PR.

mirkoperillo · 2019-10-09T14:26:40Z

@lemoncurry now gradle check ends without errors

jmrunkle · 2020-04-12T20:56:24Z

exercises/say/.meta/src/reference/java/Say.java

+        String sentence = "";
+
+        if (digits.length == 3) {
+            digits = filterInitialZeros(digits);


Reassigning method parameters is not a great practice to be encouraging in our canonical implementation.

Is it not common to prefix method parameters with final to discourage exactly this? E.g.

private String sayDigits(final char[] digits, final int orderNumber) { ...

(I'm a functional programmer and have learned Java from functional programmers, so I admit to my own insanity wrt. knowing things at compile-time.)

Annoyingly, there is a conflict here with the track policies:

Avoid using final in user-facing code. Liberal use of the final keyword remains controversial. It adds little value to test suites, and if included in starter implementations, places implicit restrictions on user solutions.

My work has more-or-less the same policy (only using the final keyword for class constants and constructor assigned member variables). Even at that, we still look at it as a bad practice to re-assign a parameter because you can lose track of the reassignment. Someone comes along later to edit the same code and does not expect the parameter to have changed so they try to check something about the input and then they debug for a while until they discover that somewhere else the input parameter has been reassigned. Principle of least surprise says don't do it.

jmrunkle · 2020-04-12T21:03:49Z

exercises/say/.meta/src/reference/java/Say.java

+        if (number < 0) {
+            throw new IllegalArgumentException("input out of range");
+        }
+        char[] digits = String.valueOf(number).toCharArray();


Rather than processing a list of characters, why not turn the input number into a long and then we can extract the numbers using numeric operations (eg. integer division and modulo)? This also gets rid of the leading zeros problem that you had to address multiple times in this solution.

jmrunkle · 2020-04-12T21:06:11Z

exercises/say/.meta/src/reference/java/Say.java

+import java.util.Arrays;
+
+public class Say {
+


General comment about this solution: I think we need a better breakdown here. This one is very chaotic and hard to follow. The instructions recommend a certain strategy that chunks things in groups of three digits. I suggest we try to have our solution mirror that suggestion.

TL;DR: If the contributor says "If it's not broken, I think this is good enough", this leads to a better reward mechanism.

Be sure to notice that the track policy on reference solutions is: "The solution does not need to be particularly great code, it is only used to verify that the exercise is coherent." There is a similar point for Haskell track on example solutions: "The example solution could be inspiration for other language implementors. It doesn't need to be perfect or very elegant. But it should be efficient enough for the test suite to finish in only a few seconds."

So while a reference solution could be better, it is okay to stop improving on it so long as it doesn't do anything "wrong" in the track interpretation of the exercise (canonical tests ± track-specific tests). If the contributor likes the mentorship of improving the reference solution, that is of course very fine.

But otherwise, it is probably better to improve on it in a separate commit, and, in general, not do that so as to discourage the tendency for reference solutions to become a gallery of well-done solutions, since we already have the (currently dysfunctional) community solutions page on the website.

I find it unusual that we officially require less rigorous code for our canonical solution than we do for our students. In particular, because the canonical solution has to be maintained afterwards by the track maintainers. For example, when the problem specifications add new canonical test cases, someone else is going to have to come along and may have to modify this code to add the new test cases.

That said, I understand that there are conflicts of interest here since we want to encourage contributions. I'm just surprised that we allow so much less rigor on our canonical implementations.

jmrunkle · 2020-04-12T21:07:55Z

exercises/say/.meta/src/reference/java/Say.java

+    private String order(int length) {
+        switch (length) {
+            case 2:
+                return "thousand";
+            case 3:
+                return "million";
+            case 4:
+                return "billion";
+            default:
+                return "";
+        }
+    }


These feel like an enum...

jmrunkle · 2020-04-12T21:19:53Z

exercises/say/.meta/src/reference/java/Say.java

+        }
+    }
+
+    private String convertUnit(char digit) {


If working with longs instead of individual characters, we would be able to use a three digit set (like 123) and then extract the hundreds, tens, and ones digits to correctly say what we want. Something like this:

private String convertNumber(long number) { // optionally - throw if number is greater than 999 int hundreds = number / 100; int tens = (number % 100) / 10; int ones = number % 10; StringBuilder sb = new StringBuilder(); if (hundreds > 0) { sb.append(convertOnes(hundreds)).append(" hundred ") } if (tens > 0) { sb.append(convertTwoDigit(number / 10)).append(" "); } else { sb.append(convertOnes(ones)).append(" "); } return sb.toString(); } String convertTwoDigit(long number) { // Optionally throw if number > 99 // Determine if we are in the 10-19 (which would use a switch-case) if (number < 20) return ... String multipleOfTen = ... // eg. "forty" int ones = number % 10; if (ones == 0) { return multipleOfTen; } return multipleOfTen + "-" + convertOnes(ones); } String convertOnes(long number) { // This one can be a simple switch-case IMO ... }

sshine

Nice of you to get back to this, @jmrunkle. Much needed.

I won't comment much on the code itself, but more on the process.

It would seem that a good strategy forward is for @mirkoperillo to decide what amount of feedback he wishes to incorporate in the reference solution. It could definitely be improved, but it shouldn't have to be perfected.

sshine · 2020-04-13T11:50:15Z

exercises/say/.meta/src/reference/java/Say.java

+import java.util.Arrays;
+
+public class Say {
+


TL;DR: If the contributor says "If it's not broken, I think this is good enough", this leads to a better reward mechanism.

Be sure to notice that the track policy on reference solutions is: "The solution does not need to be particularly great code, it is only used to verify that the exercise is coherent." There is a similar point for Haskell track on example solutions: "The example solution could be inspiration for other language implementors. It doesn't need to be perfect or very elegant. But it should be efficient enough for the test suite to finish in only a few seconds."

So while a reference solution could be better, it is okay to stop improving on it so long as it doesn't do anything "wrong" in the track interpretation of the exercise (canonical tests ± track-specific tests). If the contributor likes the mentorship of improving the reference solution, that is of course very fine.

But otherwise, it is probably better to improve on it in a separate commit, and, in general, not do that so as to discourage the tendency for reference solutions to become a gallery of well-done solutions, since we already have the (currently dysfunctional) community solutions page on the website.

sshine · 2020-04-13T11:56:40Z

exercises/say/.meta/src/reference/java/Say.java

+        String sentence = "";
+
+        if (digits.length == 3) {
+            digits = filterInitialZeros(digits);


Is it not common to prefix method parameters with final to discourage exactly this? E.g.

private String sayDigits(final char[] digits, final int orderNumber) { ...

(I'm a functional programmer and have learned Java from functional programmers, so I admit to my own insanity wrt. knowing things at compile-time.)

jmrunkle · 2020-04-13T14:40:58Z

Nice of you to get back to this, @jmrunkle. Much needed.

I won't comment much on the code itself, but more on the process.

It would seem that a good strategy forward is for @mirkoperillo to decide what amount of feedback he wishes to incorporate in the reference solution. It could definitely be improved, but it shouldn't have to be perfected.

Fair enough. I was not aware that we officially require so much less of our canonical / reference implementations.

mirkoperillo · 2020-04-14T13:34:50Z

Hi guys, thank you for the review , I will try to integrate your suggestions ASAP

jmrunkle · 2020-04-26T14:52:04Z

config.json

+      "slug": "say",
+      "uuid": "3c76a983-e689-4d82-8f1b-6d52f3c5434c",
+      "core": false,
+      "unlocked_by": null,


I did not notice this earlier, but this should be unlocked by one of the core exercises. For difficulty 3, I would suggest either one of the difficulty 2 or 3 exercises should unlock this one.

jmrunkle · 2020-04-26T21:03:06Z

Hi guys, thank you for the review , I will try to integrate your suggestions ASAP

@mirkoperillo - Any chance you are going to get around to this soonish?

msomji

LGTM

mirkoperillo added 2 commits October 6, 2019 22:24

add folder structure for say exercise

944b250

content for say exercise

da7cc92

mirkoperillo changed the title ~~[WIP] add say exercise~~ implement say exercise Oct 7, 2019

mirkoperillo marked this pull request as ready for review October 7, 2019 12:11

add README and config.json modifications for say exercise

50137e7

fix style

0d63723

yawpitch mentioned this pull request Nov 10, 2019

Make it more clear which exercises can't be implemented for Java #1589

Closed

jmrunkle reviewed Apr 12, 2020

View reviewed changes

sshine reviewed Apr 13, 2020

View reviewed changes

jmrunkle reviewed Apr 26, 2020

View reviewed changes

mirkoperillo added 2 commits November 4, 2020 14:26

simplify reference solution

03c7cfc

Merge branch 'master' into issue_1752

a910064

mirkoperillo requested review from a team and msomji November 4, 2020 13:37

msomji approved these changes Nov 6, 2020

View reviewed changes

mirkoperillo merged commit 110501c into exercism:master Nov 6, 2020

mirkoperillo deleted the issue_1752 branch November 6, 2020 13:34

Uh oh!

Conversation

mirkoperillo commented Oct 6, 2019 • edited by jmrunkle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirkoperillo commented Oct 7, 2019

Uh oh!

lemoncurry commented Oct 7, 2019

Uh oh!

mirkoperillo commented Oct 8, 2019

Uh oh!

lemoncurry commented Oct 9, 2019

Uh oh!

mirkoperillo commented Oct 9, 2019

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!

sshine 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!

jmrunkle commented Apr 13, 2020

Uh oh!

mirkoperillo commented Apr 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrunkle commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msomji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mirkoperillo commented Oct 6, 2019 •

edited by jmrunkle

Loading

jmrunkle commented Apr 26, 2020 •

edited

Loading