Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove starter implementations #681

Merged
merged 26 commits into from
Jul 6, 2017

Conversation

redshirt4
Copy link
Contributor

@redshirt4 redshirt4 commented Jul 4, 2017

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

Copy link
Member

@Smarticles101 Smarticles101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😜

@@ -1,21 +0,0 @@
public enum Plant {
Copy link
Member

@Smarticles101 Smarticles101 Jul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@@ -1,21 +0,0 @@
public enum Plant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

@@ -23,43 +23,43 @@ public void setUp() {

@Test
public void testZeroStepsRequiredWhenStartingFrom1() {
assertEquals(0, collatzCalculator.computeStepCount(1));
// assertEquals(0, collatzCalculator.computeStepCount(1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

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

@Smarticles101 Smarticles101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good now, thanks :)

@redshirt4
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

@Smarticles101 Smarticles101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now

@@ -3,6 +3,6 @@

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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@@ -1,31 +0,0 @@
class MatrixCoordinate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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?

@exercism exercism deleted a comment from FridaTveit Jul 5, 2017
@Smarticles101
Copy link
Member

Smarticles101 commented Jul 5, 2017

@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)

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

@FridaTveit changes pushed to address review kickbacks

Copy link
Member

@Smarticles101 Smarticles101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Smarticles101 merged commit ef200ce into exercism:master Jul 6, 2017
@redshirt4 redshirt4 deleted the RemoveStarterImplementations branch July 6, 2017 02:51
@redshirt4 redshirt4 restored the RemoveStarterImplementations branch July 6, 2017 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants