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

Scrabble: Add stub, remove object-oriented cruft #241

Merged
merged 8 commits into from
May 3, 2019
Merged

Scrabble: Add stub, remove object-oriented cruft #241

merged 8 commits into from
May 3, 2019

Conversation

sshine
Copy link
Contributor

@sshine sshine commented Dec 6, 2018

It does not make sense to wrap a word in a class just to call a method
on it. It is better to teach object-oriented principles by examples that
warrant the use of classes.

  • Rename expected file from Word to Scrabble: When it is
    object-oriented, the entity's name makes sense to be Word.

  • In the example solution, handle zero case with List::Util::sum0.

  • Add stub with arguments. The case my ($word, %extensions) = @_ is
    not something new Perl programmers will infer, and it is something
    they will learn from seeing.

It does not make sense to wrap a word in a class just to call a method
on it. It is better to teach object-oriented principles by examples that
warrant the use of classes.

- Rename expected file from `Word` to `Scrabble`: When it is
  object-oriented, the entity's name makes sense to be `Word`.

- In the example solution, handle zero case with `List::Util::sum0`.

- Add stub with arguments. The case `my ($word, %extensions) = @_` is
  not something new Perl programmers will infer, and it is something
  they will learn from seeing.
@m-dango
Copy link
Member

m-dango commented Dec 6, 2018

I agree with your reasoning, and I typically do the same when updating exercises myself.

For updating an exercise, I would recommend using the generator in the bin directory (it looks like I mentioned it in the perl 6 readme but forgot about the perl 5 one unfortunately!). roman-numerals may be a good example to take a look at.

@sshine
Copy link
Contributor Author

sshine commented Dec 7, 2018

Thanks!

@sshine
Copy link
Contributor Author

sshine commented Apr 17, 2019

Hi again @mienaikage

We're using some of the Exercism exercises for training new hires at work, and so I've wanted to update another exercise. Coming back to the track, I realize that some of those changes I want to push would end up in the same state as this PR, which I'd forgotten about.

I looked at the Perl 6 generator README now and see that what's missing in both the case of this PR and in the case of a similar PR I intend to submit for custom-set, is a .meta/exercise-data.yaml file.

Do I understand correctly that before I can push a change to an exercise that doesn't yet have a .meta/exercise-data.yaml, I must rewrite the existing test file into this format such that the generator can produce the .t file?

@m-dango
Copy link
Member

m-dango commented May 2, 2019

If the change is to address a bug in the exercise then adding it to the generator is not necessary, but for updates and improvements (especially if canonical data is being included) then including changes to incorporate the generator is preferred.

I've done very little on updating exercises on this track sadly, I'm currently slowly chipping away at the test2-more branch when I have the time to update the testing framework (which is mostly complete but lacking sufficient documentation to merge into master yet).

I pushed a commit to address some syntax errors, it looks like the signatures are all that need to be addressed here, I shall comment on #286 as you've raised that there.

sshine and others added 2 commits May 3, 2019 10:47
This makes the CI tests work for Perl 5.18 and below.
@sshine
Copy link
Contributor Author

sshine commented May 3, 2019

Thanks. :-)

@m-dango m-dango merged commit d4014ab into exercism:master May 3, 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

Successfully merging this pull request may close these issues.

2 participants