Binary search #186

javaeeeee · 2016-11-25T18:26:14Z

Added the binary search exercise to the Java track.

stkent · 2016-11-26T14:39:54Z

@javaeeeee thanks for this addition! Overall the exercise implementation looks very solid; I'll comment inline with a few comments/questions and we can discuss from there :)

stkent · 2016-11-26T14:42:25Z

exercises/binary-search/build.gradle

+
+dependencies {
+  testCompile "junit:junit:4.12"
+  testCompile "org.assertj:assertj-core:3.2.0"


In general, my preference (moving forward) would be for all new exercise implementations to rely on the default JUnit assertions. While I definitely think AssertJ assertions read better, it's another API surface that newcomers need to explore/understand to make sense of the tests we present.

@exercism/java thoughts on this? Is there somewhere we can capture standards such as the one I'm proposing for future reference? Thanks!

Agreed, @stkent for the exact reason you gave.

Hello, also agree. Actually used jUnit assertions in the test. In some other projects AssertJ is used, so It looks like a standard for the project. Removed the dependency.

You are right, @javaeeeee, we haven't completely removed "not technically necessary" third party dependencies. Sorry for the confusion that caused.

stkent · 2016-11-26T14:43:13Z

exercises/binary-search/src/example/java/BinarySearch.java

+
+public class BinarySearch<T extends Comparable<T>> {
+
+    public static String ARRAY_MUST_BE_SORTED = "Array should be sorted.";


This field can be marked final to make it an honest-to-goodness constant :)

Sorry, missed this. Static analyzers had several other complaints including Javadocs, so didn't spot it. Are Javadocs actually allowed in the project?

No problem! As far as I know, adding Javadocs to example implementations (when appropriate) should be just fine. Pinging @exercism/java for confirmation of that though.

Hello, Stuart, thank you for the answer. So the takeaway is that comments may be used only in the example implementations, not the tests. Am I right?

I think comments are just fine in the tests too - as mentioned somewhere else in this PR, we have them when JUnit rules are implemented, so that users can find relevant context more quickly. However, as is generally considered good practice, we definitely don't want comments on every line of code just for the sake of it ;)

In general, the code should be self-explanatory will little need for comment.

If one is tempted to add a comment, ask first, "is there a way I can reword this code to make it so clear, a comment is no longer necessary?"

Comments are best used to explain unusual/surprising context... that is to say, they share why a chunk of code is shaped in the way it is.

For example, in robot-simulator, @stkent smartly overloaded Object::equals instead of overriding it and didn't implement Object::hashCode. He left a comment explaining why and how one would do it in the real world.

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

exercises/binary-search/src/test/java/BinarySearchTest.java

+        assertEquals(actual, SORTED_LIST);
+    }
+
+    @Ignore


Thanks for ignoring all but the first test by default! Implementors thank you.

stkent · 2016-11-26T14:50:54Z

exercises/binary-search/src/test/java/BinarySearchTest.java

+        final int actual = sut.indexOf(number);
+        final int expected = -1;
+        assertEquals(expected, actual);
+    }


The binary-search problem has associated canonical test data located here.

In general, when implementing a new exercise in a track we should reference those canonical test cases (if they exist) to ensure consistency across languages. I think you have most of the expected cases covered, but for future maintainability it would be awesome if you could adjust your tests to match the canonical naming/data exactly. Thanks!

Okay. Thank you for the reference.

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

exercises/binary-search/src/test/java/BinarySearchTest.java

+            );
+
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();


Nice use of Rules! Since this is a relatively advanced concept that we've only just begun integrating into the track, we've been marking all usages with a helpful comment (for an example, see https://github.com/exercism/xjava/blob/master/exercises/queen-attack/src/test/java/QueenAttackCalculatorTest.java#L11-L14). Please add that same comment here. Once we have established a progression of exercises per #142, then we'll likely reduce this redundancy some, but better safe than sorry for now.

Removed the @rule from the tests altogether as according to the canonical test data one shouldn't test if the array is sorted.

javaeeeee · 2016-11-27T12:01:40Z

Hello, Stuart and thank you.

…

On Sat, Nov 26, 2016 at 9:39 AM, Stuart Kent ***@***.***> wrote: @javaeeeee <https://github.com/javaeeeee> thanks for this addition! Overall the exercise implementation looks very solid; I'll comment inline with a few comments/questions and we can discuss from there :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIezh3U1iyAAGnUMP_q-KsDfJjqMlF0vks5rCES7gaJpZM4K8po3> .

stkent · 2016-11-29T01:55:44Z

exercises/binary-search/src/example/java/BinarySearch.java

+
+import java.util.List;
+
+public class BinarySearch<T extends Comparable<T>> {


Just noticed that this generic type means the example implementation is a lot more general than the tests require. @exercism/java is this a good feature in a reference solution, or does it veer towards YAGNI territory?

Personally, I currently give wide berth to implementors in what happens in the example. As long as it passes the tests and represents a true solution, it's generally been implementor's choice.

I think this way because practitioners don't see these. The high value conversations in PRs are about how the test suite flows, the API decisions therein and whether we're implementing the exercise correctly.

Hello, and thank you for your comments. Here is a question: Do we need .keep files? Spotted that some were removed in one of the commits.

Since there are other files in exercises/binary-search/src/main/java/ and exercises/binary-search/src/test/java/, it should be safe to remove exercises/binary-search/src/main/java/.keep and exercises/binary-search/src/test/java/.keep. Thanks for checking!

stkent · 2016-11-29T15:59:05Z

One more merge with master needed!

javaeeeee · 2016-11-30T14:01:30Z

Merged!

stkent · 2016-12-01T17:13:15Z

Sorry for the silence yesterday; I should be able to take another look at this tonight :o)

javaeeeee · 2016-12-01T17:25:07Z

It's Okay. Thank you. There is no hurry.

stkent · 2016-12-02T14:10:06Z

Looks good! One more merge, then I'll hit the button as fast as possible so we don't need any more :)

jtigger · 2016-12-02T14:22:04Z

Take your time; I'll make sure not to merge until this one goes through (assuming no corrective feedback).

javaeeeee · 2016-12-02T15:05:00Z

Thank you. Done.

…

On Fri, Dec 2, 2016 at 9:22 AM, John Ryan ***@***.***> wrote: Take your time; I'll make sure not to merge until this one goes through (assuming no corrective feedback). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIezh2MW0vnkc02CNwCOVyB4sCNfQl9hks5rECmMgaJpZM4K8po3> .

stkent · 2016-12-02T15:10:55Z

@jtigger hah, yes, I wouldn't really race, but thank you for holding off on others. (I haven't dealt with my own merge conflict for that same reason :)

javaeeeee · 2016-12-02T16:00:07Z

Thank you. :)

…

On Fri, Dec 2, 2016 at 10:46 AM, Stuart Kent ***@***.***> wrote: Merged #186 <#186>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIezhzx1ISDCp9dmd6u_9EZxqBhTtvKTks5rED1QgaJpZM4K8po3> .

javaeeeee added 3 commits November 25, 2016 10:59

Added or modified all required files

572d9b2

Added binary search exercise

1f24f33

Corrected merge conflict in config.json

ad84b8b

stkent self-assigned this Nov 26, 2016

stkent reviewed Nov 26, 2016

View reviewed changes

Implemented recommended changes

1095210

stkent reviewed Nov 29, 2016

View reviewed changes

javaeeeee added 2 commits November 29, 2016 09:55

Merged upstream/master and updated config.json

9cca5f5

Removed a dangling comma in config.json

be31f4c

Merged config.json

ac7421e

Merged with upstream and merged conflicts

e8b4dff

Merged with upstream and resolved conflicts in config.json

1d8fa2c

stkent merged commit a47d37e into exercism:master Dec 2, 2016

stkent mentioned this pull request Dec 29, 2016

binary-search;binary-search-tree;custom-set;linked-list;simple-linked-list: use generics? #230

Closed


		public class BinarySearch<T extends Comparable<T>> {

		public static String ARRAY_MUST_BE_SORTED = "Array should be sorted.";


		import java.util.List;

		public class BinarySearch<T extends Comparable<T>> {

Uh oh!

Binary search #186

Binary search #186

Uh oh!

Conversation

javaeeeee commented Nov 25, 2016

Uh oh!

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

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!

javaeeeee commented Nov 27, 2016 via email

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 29, 2016

Uh oh!

javaeeeee commented Nov 30, 2016

Uh oh!

stkent commented Dec 1, 2016

Uh oh!

javaeeeee commented Dec 1, 2016

Uh oh!

stkent commented Dec 2, 2016

Uh oh!

jtigger commented Dec 2, 2016

Uh oh!

javaeeeee commented Dec 2, 2016 via email

Uh oh!

stkent commented Dec 2, 2016

Uh oh!

javaeeeee commented Dec 2, 2016 via email

Uh oh!

Uh oh!