Remove starter implementations by redshirt4 · Pull Request #681 · exercism/java

redshirt4 · 2017-07-04T02:54:15Z

Began implementing changes for Issue #554, currently incomplete.

I would appreciate an in-progress review to ensure implementation matches coding standard. I tried to follow the spirit of the starter implementation policy, but am unsure if empty classes are considered a stub.

Reviewer Resources:

Track Policies

Smarticles101

Thanks for taking this on.

You should delete the entire file that has the starter implementation instead of just deleting the method stubs. Once you delete the file with the starter implementation, add a .keep file in it's place.

Also, tests don't need to be commented out.

Good luck 😜

Smarticles101 · 2017-07-04T03:09:52Z

exercises/kindergarten-garden/src/main/java/Plant.java

@@ -1,21 +0,0 @@
-public enum Plant {


I'm pretty sure this file needs to stay here as it is needed to complete the exercise. Thoughts @FridaTveit ?

Yes, this should still be here. When we use enums we usually include this in the starter implementation regardless of the difficulty of the exercise. This is because they're not really part of the solution to the problem, they're more part of the problem specification if that makes sense :) Sorry for the confusion @redshirt4! :)

FridaTveit

Thank you for helping with this @redshirt4! :) A few changes needed, but that's mostly because we changed out mind about starter implementations just now! Sorry about that :)

FridaTveit · 2017-07-04T17:21:19Z

exercises/kindergarten-garden/src/main/java/Plant.java

@@ -1,21 +0,0 @@
-public enum Plant {


Yes, this should still be here. When we use enums we usually include this in the starter implementation regardless of the difficulty of the exercise. This is because they're not really part of the solution to the problem, they're more part of the problem specification if that makes sense :) Sorry for the confusion @redshirt4! :)

FridaTveit · 2017-07-04T17:26:27Z

exercises/collatz-conjecture/src/test/java/CollatzCalculatorTest.java

    @Test
    public void testZeroStepsRequiredWhenStartingFrom1() {
-        assertEquals(0, collatzCalculator.computeStepCount(1));
+//        assertEquals(0, collatzCalculator.computeStepCount(1));


As per the discussion in PR #683, we decided not to comment out tests after all as this might be confusing to people solving the exercise. The POLICIES doc has been updated to reflect this. So all you have to do is to remove the CollatzCalculator.java file completely and replace it with a .keep file (the .keep file basically tells git not to ignore that directory even though it's empty). Sorry for the confusion! :)

redshirt4 · 2017-07-04T18:28:07Z

Thank you @Smarticles101 and @FridaTveit for the clarifications. I'll take a look at the new spec and push.

Smarticles101

These look good now, thanks :)

redshirt4 · 2017-07-05T17:47:15Z

Thank you for the review @Smarticles101 and @FridaTveit . Is it best practice to ask for a re-review of current, or to open a new pull request?

Smarticles101 · 2017-07-05T18:03:23Z

@redshirt4 I think it is best practice to ask for a re-review or to wait for another review.

Smarticles101

Looks good now

Smarticles101 · 2017-07-05T18:05:23Z

exercises/etl/src/main/java/Etl.java

 public class Etl {
    public Map<String, Integer> transform(Map<Integer, List<String>> old) {
-        return null;
+        throw new UnsupportedOperationException("Delete this statement and write your own implementation.");


The fact that you didn't delete this starter implementation because it had a complex stub shows that you read and paid attention to the policies doc which means alot

FridaTveit

This is looking really good! Thank you for tackling all of these @redshirt4! There's just one file which shouldn't be removed and then you're good to go :) Great work!

FridaTveit · 2017-07-05T18:30:01Z

exercises/saddle-points/src/main/java/MatrixCoordinate.java

@@ -1,31 +0,0 @@
-class MatrixCoordinate {


Please don't remove this file. Like the occasional enum definitions and exception classes in other exercises, this has been provided as part of the solution so that the exercise isn't overly complicated to implement :) You also don't need a .keep file if you keep this in.

FridaTveit · 2017-07-05T18:46:17Z

Sorry @Smarticles101, I didn't notice you were reviewing this at the same time! I was trying to do the review on a train which only occasionally has internet :P I don't think it matters very much about MatrixCoordinate. But since it requires overriding equals and hashCode it might be nice to leave it in to reduce complexity, especially since saddle-point only has difficulty 4. What do you think?

Smarticles101 · 2017-07-05T18:50:03Z

@FridaTveit I agree. I missed that file on accident. But yes, it should be kept. (You also double posted, so I deleted the second one)

- Classes as a whole removed, not just method/constructor stubs - Commenting out tests undone per discussion, exercism#683 - Enum 'Plant' re-added to Kindergarten Garden per discussion - .keep added to empty source folders

…ies. bob, pascals triangle correct as is. bracket push stubs removed

…s. beer song stubs removed.

… with starter throw.

…enum and very specific Grid Position with non-standard equals, leaving those in while removing other stub.

…ption in place. anagram stubs removed

…s removed.

…complexity.

redshirt4 · 2017-07-06T01:32:30Z

@FridaTveit changes pushed to address review kickbacks

Smarticles101

I went over everything and it seems like you got everything in order and it's all how it should be, so I'd say it's ready to merge. Thanks for all the effort you put into this @redshirt4 😄

Smarticles101 reviewed Jul 4, 2017

View reviewed changes

FridaTveit suggested changes Jul 4, 2017

View reviewed changes

redshirt4 force-pushed the RemoveStarterImplementations branch from 121dcd8 to 9629a4b Compare July 4, 2017 18:46

Smarticles101 reviewed Jul 4, 2017

View reviewed changes

Smarticles101 approved these changes Jul 5, 2017

View reviewed changes

FridaTveit suggested changes Jul 5, 2017

View reviewed changes

exercism deleted a comment from FridaTveit Jul 5, 2017

redshirt4 added 17 commits July 5, 2017 21:31

Removed KindergartenGarden stubs, commented out tests.

a963110

Removed Collatz-Conjecture stubs and commented out tests.

4a8540d

Addressed pull request comments:

0deaff1

- Classes as a whole removed, not just method/constructor stubs - Commenting out tests undone per discussion, exercism#683 - Enum 'Plant' re-added to Kindergarten Garden per discussion - .keep added to empty source folders

nth prime correct as is. saddle point stubs removed.

4194008

diamond stubs removed

1e69bcf

isogram correct as is. flatten array stubs removed

9420b7a

pig-latin and phone number correct as is. nucleotide count stubs removed

fa13279

word count stubs removed

9329646

run length encoding stubs removed

3a41b31

robot name, prime factors correct as is. Allergen enum left on allerg…

02e2288

…ies. bob, pascals triangle correct as is. bracket push stubs removed

series stubs removed

db4a98b

atbash cipher, roman numerals, transpose, and food chain correct as i…

ba2ee1a

…s. beer song stubs removed.

queen attack stubs removed

e677e6f

etl has a complicated method structure for its stub, leaving in place…

fd12319

… with starter throw.

linked list, grade school correct as is. Robot Simulator provides an …

7c8b90c

…enum and very specific Grid Position with non-standard equals, leaving those in while removing other stub.

binary search stubs removed

d1fb762

minesweeper stubs removed

5688a18

redshirt4 added 9 commits July 5, 2017 21:31

wordy stubs removed.

02d2ada

all your base stubs removed

a74681d

bank account has user exercising concurrency, left provided bank exce…

4e5e9db

…ption in place. anagram stubs removed

sublist stubs removed, enum left in place

9967145

Clock correct as is, OCR reader stubs removed

ee5e399

rectangles stubs removed

d5d1c71

book store, simple cipher, complex numbers correct as is. change stub…

573a739

…s removed.

palindrome products correct as is, pythagorean triplet stubs removed

c05a2d5

Review 2 changes - saddlepoint's MatrixCoordinate re-added to reduce …

e8c52fb

…complexity.

redshirt4 force-pushed the RemoveStarterImplementations branch from 742cd6a to e8c52fb Compare July 6, 2017 01:31

Smarticles101 approved these changes Jul 6, 2017

View reviewed changes

Smarticles101 merged commit ef200ce into exercism:master Jul 6, 2017

Smarticles101 mentioned this pull request Jul 6, 2017

Remove starter implementations for exercises with positions 21 or greater #554

Closed

redshirt4 deleted the RemoveStarterImplementations branch July 6, 2017 02:51

redshirt4 restored the RemoveStarterImplementations branch July 6, 2017 02:55

This was referenced Jul 6, 2017

diamond: create and/or validate starter implementations #553

Closed

nth-prime: create and/or validate starter implementations #552

Closed

Uh oh!

Conversation

redshirt4 commented Jul 4, 2017 • edited by stkent Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Smarticles101 Jul 4, 2017 • 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!

FridaTveit Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

FridaTveit Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

redshirt4 commented Jul 4, 2017

Uh oh!

Smarticles101 left a comment

Choose a reason for hiding this comment

Uh oh!

redshirt4 commented Jul 5, 2017

Uh oh!

Smarticles101 commented Jul 5, 2017

Uh oh!

Smarticles101 left a comment

Choose a reason for hiding this comment

Uh oh!

Smarticles101 Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

FridaTveit Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

FridaTveit commented Jul 5, 2017

Uh oh!

Smarticles101 commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redshirt4 commented Jul 6, 2017

Uh oh!

Smarticles101 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

redshirt4 commented Jul 4, 2017 •

edited by stkent

Loading

Smarticles101 left a comment •

edited

Loading

Smarticles101 Jul 4, 2017 •

edited

Loading

Smarticles101 commented Jul 5, 2017 •

edited

Loading