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

Created New Exercise: Retree #1424

Merged
merged 11 commits into from
Jan 8, 2019
Merged

Created New Exercise: Retree #1424

merged 11 commits into from
Jan 8, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 30, 2018

The task is to rebuild binary trees using pre-order and in-order traversals of the original tree.
Possible tags for this exercise include: recursion, binary trees, and tree traversals.

@sshine
Copy link
Contributor

sshine commented Dec 31, 2018

Interesting exercise! Does it have a discussion, or is this the first place to discuss it?

Here are some points:

  1. "Reforestation" is a nice, creative name, but there aren't any forests in the main exercise, only trees. (On a related tangent, I'd like to rename the "tree-building" exercise, as it is not creative. Most tree-based exercises are about building.)

  2. I haven't read the exercise text in the rendered version (only the Markdown code), but perhaps this is still a little difficult? This seems like it has been cut out of an algorithms textbook. Perhaps a link to source material for the algorithm(s) that apply? Maybe someone else can chime in here.

  3. Not having solved it, perhaps I'm wrong here, but: isn't there very few ways to solve this exercise? This is a less attractive property of advanced exercises, I think.

Also, I'm happy to see more tree-based exercises as I've been thinking loud in #1388.

@sshine
Copy link
Contributor

sshine commented Dec 31, 2018

Would it make sense to phrase the exercise with a theme, so that it sounds less like a hunt for an existing algorithm that one can mindlessly transcribe from pseudocode?

As in, if you recognize the general pattern or somehow extract it, that's perfect, but not what's primarily encouraged.

@ghost
Copy link
Author

ghost commented Dec 31, 2018

Yup, this is the first place for discussion..

  1. True. "retree" was my other option. "tree decompression" might work, too.

  2. Do you mean difficult to read? The definitions are from my mind, but I did read them in a text book.

  3. I know of one algorithm which I am sure can be implemented in different ways.

My original intention was to add this exercise to the Prolog track. In Prolog you can implement the algorithm directly or use DCGs. Currently there are no exercises that encourage using DCGs so I thought I should add one. This exercise lets Declarative Prolog show off its best qualities, but also some of its shortcomings (i.e. not purely declarative). In short DCGs would be the theme I suggest.

The Exercism docs recommend creating an exercise in this repo first. For most languages I can see this being simply "transcribing pseudocode." But in Prolog it could be useful.

Is it allowable to create an exercise specifically for a language track without a problem-specifications entry?

@sshine
Copy link
Contributor

sshine commented Jan 1, 2019

  1. "Retree" is also a good, creative name. I kind of like it better because it sounds like "retry", but other than that, the only reason why I want something other than "Reforestation" is that I'd like an exercise with forests. :-)

  2. Yes, difficult to figure out how to solve. I suspect. I don't know what would be the right amount of guidance, though. The "transcribing pseudocode" critique is mostly if there's really one way to do it, and the average student ends up googling it (which I suspect is more likely when the exercise is phrased in the same way as the solutions online). An example I can think of is "difference-of-squares".

Wrt. language-specific tracks track-specific exercises, yes, I think so. See e.g. the Ballerina track's exercises. But I really don't think it is a good approach in general. Prolog might be an exception because it's borderline not-general-purpose.

In this case, for example, I'd welcome more tree-based exercises in the ML-style tracks, so we can share the exercise design process.

One last comment on this exercise text proposal: Either remove the "you can generate multiple results" paragraph from the description, or add a test case that demonstrates multiple results. I would remove the paragraph and add a "hint" (extra section of the README on the language track) and a track-specific test for the multiple results: The exercise without duplicates are already difficult and interesting, and the "multiple solutions as a side-effect" is a Prolog thing. :)

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I like this exercise, it is a welcome addition to the existing body of exercises. Great work! I've left some comments.

exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
exercises/reforestation/canonical-data.json Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 4, 2019

@sshine I have been thinking about how to address your suggestions. I tried my best. "Bonus points" section of the README and the corresponding test were removed. I have added a little story to make the exercise less dry. Finally I added a small note at the end. I think it is just the right size hint. Big enough to encourage exerciser not to google, but small enough not to spoil the exercise.

@ErikSchierboom do you resolve conversations or do I? I'm not familiar with etiquette for this feature.

@ghost ghost changed the title Created New Exercise: Reforestation Created New Exercise: Retree Jan 4, 2019
Copy link
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Excellent story format! We're nearly there, I think.

I think your explanation of pre-order and in-order are sufficient, and if not, I think the Wikipedia link will discourage people from accidentally finding the complete solutions that are available in C, Java, Python and C# on this google search giving this page.

Should there be a test case that fails for duplicate elements, or should the function assume that there are no duplicates? We already have two negative classes of input that are easier to test; having this would be more complete, but would also introduce complexity in the exercise. I think in the case of Prolog you'd probably want to assume the input has unique elements. -- Maybe another test case that is optional to implement?

Thank you very much for your work so far. :-)

exercises/retree/canonical-data.json Outdated Show resolved Hide resolved
exercises/retree/canonical-data.json Outdated Show resolved Hide resolved
exercises/retree/description.md Outdated Show resolved Hide resolved
exercises/retree/description.md Show resolved Hide resolved
exercises/retree/description.md Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Two small nits. As for resolving, that happens automatically I believe.

exercises/retree/canonical-data.json Outdated Show resolved Hide resolved
exercises/retree/canonical-data.json Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 4, 2019

Added a test for unique items since I don't think it adds too much complexity. And it helps point out subtle details.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Looking great! Just one final nit.

"expected": {"error": "traversals must have the same length"}
},
{
"description": "Reject inconsistent traversals of same length",
Copy link
Member

Choose a reason for hiding this comment

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

I think the indentation of this test case is off (too much to the right).

Copy link
Contributor

Choose a reason for hiding this comment

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

Other than that and we're ready to merge.

We should have a linting rule for this.

Copy link
Member

Choose a reason for hiding this comment

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

We should have a linting rule for this.

Yes, we should!

@petertseng

This comment has been minimized.

@sshine
Copy link
Contributor

sshine commented Jan 7, 2019

A tangential pun: If we are ever to colonize the stars, eventually we'll want to send send trees in serialized form for terraforming purposes. :-D

@sshine sshine merged commit 0c769d4 into exercism:master Jan 8, 2019
@sshine
Copy link
Contributor

sshine commented Jan 8, 2019

I look forward to implementing this exercise for Haskell, Ocaml and SML. :-)

@ErikSchierboom
Copy link
Member

Great work @fidelcoria! Thanks for being patient with us reviewers :)

@ghost
Copy link
Author

ghost commented Jan 8, 2019

Thank you! This is my first Open Source contribution. This was a good experience!

@ErikSchierboom
Copy link
Member

This is my first Open Source contribution.

You should be proud of yourself then! This is high quality first pull request.

@iHiD
Copy link
Member

iHiD commented Feb 1, 2019

I've made a suggestion about renaming this exercise at #1451 :)

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.

6 participants