Skip to content

Change Minimal Solution for Acronym #943

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

Closed
emcoding opened this issue Feb 28, 2019 · 23 comments
Closed

Change Minimal Solution for Acronym #943

emcoding opened this issue Feb 28, 2019 · 23 comments

Comments

@emcoding
Copy link
Contributor

Currently, the Minimal Solution for Approval for Acronym as advertised in the Mentor Notes is:
fullname.scan(/\b[a-zA-Z]/).join.upcase

in favour of solutions that use split.
As different people has pointed out, this solution is not ideal.

  • It needs a Regular Expression, which is not a primary goal for the Ruby track, and it's extra non-elegant that the first real exercise in the track requires a regex.
  • Its advantage that it 'focusses on what you need, instead of naming all the delimiters' is debatable. As @pgaspar (and others too) pointed out to me: it's much easier to add a new delimiter than to re-write the regex to match a new pattern.

I know the Mentor Notes state that the exercise is meant to introduce scan - I wrote that myself - but the reason behind it was rather: showing the strength of Ruby's built in methods, and showing that Ruby has a lot of convenience methods for common operations. That's supercool, but scan is not the best showcase in this regard.

So, with the new insights, I'm looking for a candidate to replace the scan solution.
This is my favorite so far:

fullname.tr('-', ' ').split.map {|word| word.chr}.join.upcase

It's a much better show case for Ruby's specialty methods, with tr for gsub and chr for Array#[] And no need for a regex, yay!
(And in case you wonder: it's faster than the scan solution. )
OTOH I'd rather introduce Symbol#to_proc later, but it may be hard to resist the temptation to show map(&:chr) for us while mentoring.

Recap:

  • Your thoughts on replacing the current scan solution
  • Your thoughts on the candidate
  • Other candidates to replace the current scan solution?
@iHiD
Copy link
Member

iHiD commented Mar 1, 2019

I have a lot of fun mentoring this exercise and have done it lots. Some thoughts:

  • It's nearly always something that I can teach someone something with (ie people don't get the right solution first time - I think this is good).
  • I think learning about scan and boundary regexps is good.
  • Nearly every solution uses split and nearly always uses a more primitive regexp - those people like the boundary upskill and generally comment positively
  • Some solutions don't use a regexp. In these cases, mentor discretion is advised. Did the person just not consider it? Are they unfamiliar with regexps? In the later case, I think it's great to introduce them to them, but not block approval on them using it. In the former case, most people quickly go away and refactor. I've never felt the regexp thing has been a problem for me.
  • I use regexps 100x more than I use any maths in programming. I think that regexps are a crucial skill for developers to learn, and do not have to be complicated or confusing if only the basic things are taught. I think there's way too much drama about regexps generally in the development community. Sure - they can be impossibly hard when you get deep into them, but \b\w is not a hard thing to learn. In fact I'd argue that adding the forward slashes makes it much harder (/\b\w/) - I'd happily suggest not using the shorthand and going for Regexp.new("\b\w") if the person is entirely new to regexps instead.
  • I really like that Word Count uses the same scan / boundary regexp sort of thing later on. It means I feel like I can reuse my knowledge

All of this makes me a big fan of this exercise - it's possibly my favourite to mentor - and I'd be sad to see it changed or watered down.

However, your points are much more important than mine. If core#3 needs to teach cool ruby methods, is this the wrong exercise to do it? The optimal solution to core#3 should be something consisting of lots of chained cool ruby methods, which the optimal solution to this isn't.

Saying that, I'm not entirely against guiding someone to the solution you pasted. But I think if we do do that, I'd like to have a rule that we generally give people a "Bonus test" at the end, which has a few more chars that aren't - in them, and challenge them to replace tr with scan to achieve this, or something. I guess I'm wary of watering things down for learners that don't need it watering down (ie, if I did this exercise and wasn't told I could use scan, even as an after-comment, I'd feel like I'd lost out).

There's a general point here as well, that there isn't an absolute optimal/guidance solution here. There's a different optimal one depending on the starting point. One great use of the Automated Mentoring Support Project might be to take a solution to Acronym and say to the mentor "This is the target solution for this student". So if they start with gsub then we push to scan. If they start with total chaos, we push them to the new proposed solution above.

@codeholic
Copy link

codeholic commented Mar 1, 2019

I agree that the purpose of Ruby track is to learn Ruby. And regular expressions are part of it.

What I would change though is to encourage developers to think about diversity of the world we're living in, so I would add a couple of tests for Unicode (maybe German with diacritics and Russian, leaving Arabic and Hebrew aside 😄) and change the minimal solution to sentence.scan(/\b[[:alpha:]]/).join.upcase

@kotp
Copy link
Member

kotp commented Mar 1, 2019

I would probably think about that diversity in a later exercise, and one that is focused on encoding, perhaps more than Regexp. Or to give as a challenge if it appears that the student is up for it in discussions.

@iHiD
Copy link
Member

iHiD commented Mar 1, 2019

Both good points. I think that teaching how to use Regexps in Ruby is important. I think teaching the details of regexpes themselves is not in scope - that doesn't mean we don't do it as a bonus thing, but it's not part of what we must do.

So I think teaching /.../ and Regexp.new(...) are good, but that we shouldn't try and get too complex in the regexp we teach. So if someone hasn't used Regexps before, we teach [[:alpha:]]over \w but we don't correct \w to [[:alpha:]] as a blocking thing.

@joshgoebel
Copy link
Contributor

joshgoebel commented Mar 1, 2019

fullname.tr('-', ' ').split.map {|word| word.chr}.join.upcase

I hate, hate, hate this solution and always try and steer students away from it. Most are super impressed when they see the simplicity of what can be done with JUST scan and a SIMPLE regex.

I find negative filtering (dealing with characters like "-" on a one off basis) is also a far more fragile programming practice that scanning for a positive match. If you need a-z, just scan a-z, don't try and filter out all the junk, because there is a lot of junk out there - you'll be updating your code every day in the real world, etc... I think that's an important lesson that this exercise gives an opportunity to teach.


Update: Almost none start with tr/split though... all the students (IIRC) are already using regex to being with, just not an optimized regex... and only they are trying to splitting on a regex vs scanning.

@mikegee
Copy link

mikegee commented Mar 1, 2019

I agree with @yyyc514, but perhaps not as strongly 😄

I prefer to phrase the code positively. scan for what we want, not split on the parts we don't want.

I also agree that we might be over-emphasizing regular expressions.

@joshgoebel
Copy link
Contributor

joshgoebel commented Mar 1, 2019

I also agree that we might be over-emphasizing regular expressions.

That for sure might be true on some exercises, but I don't think this one. It sure would be nice if the exercises could give students one or two small hints though. Like a hint or link to "word boundaries" would help so many students get this right the first time around I think.

The only thing simpler than \b\w is \w, and now that's a single expression regex.

I mean I supposed you have to understand the concept of grouping/meta-characters, etc... but students almost always know that already... they are trying to use \w in a 100 zany ways to solve this. The only piece of the puzzle that are missing is \b (at least in my experience).

@pgaspar
Copy link
Member

pgaspar commented Mar 2, 2019

Here are my thoughts:

I love the idea of focusing on the things you want vs. focusing on the things you want to exclude. scan allows you to do just that and I found myself using/suggesting it in later exercises as well. From my experience students usually really appreciate the change in perspective, and that's awesome!

Recently I've had a few students coming from other tracks ask me if scan really is the best option here if you start thinking about some of the tests we are purposefully leaving out in the Ruby track. Here's one of those interactions. One of these tests - the most relevant here - uses the "Halley's Comet" string.

In my experience, changing the scan solution to make it return "HC" instead of "HSC" as the test requires is very challenging - I spent some time on it once and ended up giving up after playing around with a few complex Regexes. On the other hand, it's much easier to pass this test with the split version - you just add ' to the list of characters you want to remove.

For this reason when students wonder outside the provided test cases I end up suggesting that a split-based solution may be a bit easier to maintain, depending on the edge cases you want to support.

I wonder if we should have something on the mentor notes talking about this. Something pointing out that while this solution is great for the tests that we have on the Ruby track it gets complicated very fast when you introduce all the test cases described on the problem specifications repo.

Does anyone know of an elegant Regex we can suggest for scan that passes these extra tests? You can see them here if you don't know what I'm talking about.

So, that's my main gripe with the minimal solution we propose now - it's an excellent improvement if you only consider the tests we have but it falls apart when you start thinking about some not-that-uncommon edge cases, which some more advanced students end up doing.

PS: personally I don't care too much about the use of a regular expression this early in the track, but I understand if it is off-putting for some students.

@joshgoebel
Copy link
Contributor

joshgoebel commented Mar 2, 2019

"Halley's Comet" .scan /\b(\w)[a-z']+/i
=> [["H"], ["C"]]

Just quickly. Might be better approach. But you're right that the exercise would get much more complex if we included such examples.

"The Road _Not_ Taken".scan /\b\s?[^a-z]?(\w)[a-z']*/i
=> [["T"], ["R"], ["N"], ["T"]]

Definitely more of a challenge. And starting to feel like the point where just splitting into "word units" and then analyzing them with regex might be a better approach for most students.

I'm all for added these more complex things to the tests though, and if that means finding a better proposed solution then so be it.

@pgaspar
Copy link
Member

pgaspar commented Mar 2, 2019

That's great, thank you ❤️ Didn't think of using () for "Halley's Comet" 😓 We should add that one to the list of regexes on the mentor notes.

This gives me a bit more maneuverability next time someone asks about that test.

For the second one, the mentor notes have this: /(?<!\p{Alpha})\p{Alpha}/

Learned a few things by studying that one... Namely the negative lookbehind assertion (?<!

@joshgoebel
Copy link
Contributor

joshgoebel commented Mar 2, 2019

Is that negative lookahead or non-capture or what? I get lost at this point, lol.

Does anyone have a clue if students actually go thru the tests one at a time removing skip as they go?

@pgaspar
Copy link
Member

pgaspar commented Mar 2, 2019

Is that negative lookahead or non-capture or what? I get lost at this point, lol.

Negative lookbehind, yep. Had no idea this existed 😄

(?<!pat) - Negative lookbehind assertion: ensures that the preceding characters do not match pat, but doesn't include those characters in the matched text

@joshgoebel
Copy link
Contributor

Ah, yeah forgot about negative look-BEHIND. LOL. Regex is ridiculous.

@pgaspar
Copy link
Member

pgaspar commented Mar 2, 2019

Some final points:

  • I really appreciate the elegance of the scan solution when compared to split solutions (for the tests we currently support), even if there's a regex involved.
  • I still think it's simpler to support the extra tests with split than with scan. But that probably shouldn't matter when it comes to the minimum viable solution, given the tests we support.
  • I'd like us to support the extra test cases, but that would make this considerably harder when using scan. And that amount of complexity doesn't make sense this early in the track.
  • I think it would be useful to add a "what if we wanted to support more edge cases" section to the mentor notes exploring some of the trade-offs and possibilities (I can help!).

@joshgoebel
Copy link
Contributor

I think it would be useful to add a "what if we wanted to support more edge cases" section to the mentor notes exploring some of the trade-offs and possibilities.

Or what about optional tests? A few of the other exercise have "This test is hard, you don't have to make it pass necessarily." Maybe something like extra credit?

@pgaspar
Copy link
Member

pgaspar commented Mar 2, 2019

That's also a nice option IMO 👍 Updating the mentor notes would be even more relevant in that case, I think, since a lot more people will be thinking about these cases.

@emcoding
Copy link
Contributor Author

emcoding commented Mar 4, 2019

So I totally agree that Aconym has excellent options to make it more interesting. It's def something to take into consideration - for the future.
Actually, there's an issue for that: exercism/problem-specifications#1463
We will def discuss (as soon as the Ruby Track Anatomy is publicly available), if we can move Acronym up and add the extra's, or, as I suggest in the other issue, to add a SuperAcronomyzer. To me, both have their merits.

At the moment, though, :

  1. We do not have enough easy exercises
  2. One effect of the Track Anatomy Project is that we will have to take into consideration how changes to one exercise affects the track.
  3. That's the reason for Temporarily not accepting changes to some exercises #935: this is not a good time to change exercises.

Also, it's not about not having regex in the track, it is about the best exercise at the best position.

So. Given the current test suite and the current position of Acronym in the track, may I rephrase my question:
Given all that, Is this the best alternative?

fullname.tr('-', ' ').split.map {|word| word.chr}.join.upcase

@codeholic
Copy link

At least fullname.tr('-', ' ').split.map(&:chr).join.upcase

@kotp
Copy link
Member

kotp commented Mar 4, 2019

As a note to mentor, I would like to have the transition explicitly commented if it is early in the track of "Why" the map(&:chr) exists which is to explicitly remind that it is exactly because the method called chr is called on each iteration, and the result (return of that call to chr) is the stored value of the map. The mentor reading the note may not, depending on experience, recognize it as doing so, or may not remember it.

@emcoding
Copy link
Contributor Author

emcoding commented Mar 4, 2019

Like we currently have in the Series notes:

- `map(&:join)`: instead of map with block, but at this point in the track it's okay to just accept it if students use it, no need to require it or dive into the subject of `Symbol#to_proc`  

@emcoding
Copy link
Contributor Author

emcoding commented Mar 4, 2019

But if I understand correctly, there's no worrying about extra objects that are created, or whatever Ruby things I may not be aware of?

@iHiD
Copy link
Member

iHiD commented Mar 5, 2019

There's a general point here as well, that there isn't an absolute optimal/guidance solution here. There's a different optimal one depending on the starting point.

I feel like this point got overlooked slightly :)

I'm strongly against guiding someone to a suboptimal solution if they're perfectly capable of guiding them to an optimal solution*. So if we do end up changing the mentor notes for this (which I think is what we're discussing really here, rather than discussing changing the exercise - and I wonder if therefore this would have been better in exercism/website-copy rather than exercism/ruby to emphasise that - should I move it?) then I'd like to explicitly say something along the lines of "If they start at X guide them to Y. If they start at Y guide them to Z and mention scan as a bonus task"

So, while it's not necessarily ideal, I would argue that it would be better to design an exercise specifically to chain the actual methods you (Maud) feel we should teach at this stage, than try to shoehorn this one in. Adding this as a side-exercise hanging off that new exercise could work nicely. Which, with reference to your comment of "showing the strength of Ruby's built in methods, and showing that Ruby has a lot of convenience methods for common operations." leads me to ask the question of "What are we actually trying to demonstrate to the student here". Or to use Track Anatomy speak, what axes is this exercise on on the progression lines?

(* I'm defining "optimal" as how I would do it :))

@emcoding
Copy link
Contributor Author

emcoding commented Mar 5, 2019

Hmmm, yeah. Okay, I'll leave it.

@emcoding emcoding closed this as completed Mar 5, 2019
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

No branches or pull requests

7 participants