Skip to content

Simplify High-scores exercise. Replace Class with functions #1764

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

Merged
merged 11 commits into from
May 3, 2019

Conversation

simmol
Copy link
Contributor

@simmol simmol commented Apr 22, 2019

As discussed with @cmccandless in #1744, i submit a PR for simplifying high score exercise.

Exercise text edited.
Tests made to expect functions. One test removed, cause was no longer valid.
Make new example.py using functions.
Add the functions as starting point in the exercise file. (This also make the tests not complain of missing imports and such.)
Added init.py to the package for the people that still run 2.7+
Added pytest cache to gitignore file

Exercise text edited.
Tests made to expect functions. One test removed, cause was no longer valid.
Make new example.py using functions.
Add the functions as starting point in the exercise file. (This also make the tests not complain of missing imports and such.)
Added __init__.py to the package for the people that still run 2.7+
Added pytest cache to gitignore file
@simmol simmol requested a review from a team as a code owner April 22, 2019 06:04
Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

Please remove init.py and see my comments below.

Remove __init__.py
Remove content of HINTS.md
Generate the README.md
Remove the comments in stub file
@simmol
Copy link
Contributor Author

simmol commented Apr 23, 2019

All Changes made.


- [**A First Look at Classes**](https://docs.python.org/3/tutorial/classes.html#a-first-look-at-classes) from the Python 3 documentation.
- [**How to Define a Class in Python**](https://realpython.com/python3-object-oriented-programming/#how-to-define-a-class-in-python) from the Real Python website.
- [**Data Structures in Python**](https://docs.python.org/3/tutorial/datastructures.html) from the Python 3 documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to ping @BethanyG for suggestions on new HINTS content. She put some good work into the current HINTS.md (less than a month ago), and I would feel bad about dumping that work without offering the chance to contribute an alternative.

Copy link
Member

@BethanyG BethanyG Apr 23, 2019

Choose a reason for hiding this comment

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

I'm happy to write up some new hints...but I think the bigger issue here is that if you remove making a class in this exercise (which is a fairly easy one), then when students have to make a class (and deal with side effect questions) in the next exercise..well, we've just kicked the can down the road. The same issues that apply to High Scores apply to Matrix (and Kindergarten Garden, etc.) ...and eventually, a python learner has to to deal with classes. They're part of the language (it's really is objects all the way down), and widely used in frameworks like Django. The sooner students get (even a little) practice in a controlled fashion, the less they struggle later.

So the way I see it is that we either have to craft an exercise or two around classes and mutability specifically, or re-work tests and mentor notes on current exercises to address it. I don't think the answer is to re-write this exercise (and the next) to not need a class. If we don't want a class-based exercise here because we want to focus on lists, we could move this exercises to a later stage (and address its shortcomings through a re-write of tests). We could replace it with something like Say or Run Length Encoding - or any other number of exercises that focus on lists and arrays. But only 6 of 18 core exercises require writing a class currently, and I'm not sure we are doing a great job of giving students practice in that area.

All that being said....if we do indeed want to make this exercise function based, I can try to make some new hints this week. However, we then should move these hints to the Matrix exercise, since it will then be the exercise to introduce classes. We maybe should also provide a hint on mutability there as well.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies - forgot one last point: We'll need to re-work the mentor notes for this exercise as well. I don't think I will be able to get to either that or the hints until this afternoon, PST.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the way I see it is that we either have to craft an exercise or two around classes and mutability specifically

I'm not opposed to that.

If we don't want a class-based exercise here because we want to focus on lists, we could move this exercises to a later stage (and address its shortcomings through a re-write of tests).

There are very few easy exercises as it is. I'd prefer not to overcomplicate what was meant to be a simple list exercise.

All that being said....if we do indeed want to make this exercise function based, I can try to make some new hints this week. However, we then should move these hints to the Matrix exercise, since it will then be the exercise to introduce classes. We maybe should also provide a hint on mutability there as well.

I can get behind that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok - I wasn't able to make suggestions in the pull (I don't have access) - but here's my proposed hint text:


In this exercise, you're going to use and manipulate lists. Python lists are very versatile, and you'll find yourself using them again and agin in problems both simple and complex.


I'll submit a pull for revised mentor notes and for the new hint on Matrix.

Copy link
Member

Choose a reason for hiding this comment

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

See exercism/website-copy#956 for the updated mentor notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @BethanyG :)
I will add the new hints right now :)

And actually will push the old hints to Matrix in this PR. So we did not loose them.

For me Matrix is better Class based exercise. But we can make other also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind creating a separate PR for the changes to matrix? We try to limit PR changes to one exercise at a time (there are few exceptions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well doesn't that count as exception? We have not changed the exercise in Matrix, just add the hints because previously they ware available at this exercise?

I can open other PR is not a big deal, but we have to merge them one after the other. I thought it would be better if we push this as one change.

I will make new one tomorrow morning and will edit this one to remove matrix docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have an issue with both exercises temporarily having the same generic hints about classes, so I went ahead and merged #1766.

Added hints for some docs around list and tuples - thanks @BethanyG for that

Moved original hints for classes in matrix ( cause is the next exercise for now which use class)
slightly edited to not refer to high-score

Regenerate both README.md
@simmol
Copy link
Contributor Author

simmol commented Apr 25, 2019

Moved changes to Matrix in separate PR: #1766
@cmccandless Hope we are good for merge now :)

Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things:

  • Typo in HINTS.md
  • A space was left in the README.md for matrix. Please remove all changes to matrix from this PR.

Should be good to merge after the above!

@simmol
Copy link
Contributor Author

simmol commented May 2, 2019

I had applied the comments. Sorry for late response but I was on vacation.

- [**A First Look at Classes**](https://docs.python.org/3/tutorial/classes.html#a-first-look-at-classes) from the Python 3 documentation.
- [**How to Define a Class in Python**](https://realpython.com/python3-object-oriented-programming/#how-to-define-a-class-in-python) from the Real Python website.
- [**Data Structures in Python**](https://docs.python.org/3/tutorial/datastructures.html) from the Python 3 documentation.
In this exercise, you're going to use and manipulate lists. Python lists are very versatile, and you'll find yourself using them again and agin in problems both simple and complex.
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to run configlet generate one more time to get Travis to pass. We should be good after that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will see to do it later today or first thing in the morning.

@simmol
Copy link
Contributor Author

simmol commented May 3, 2019

@cmccandless I think this is all for this PR :)
Lets merge it and continue with other things that need to be fixed/improved :)

@cmccandless cmccandless merged commit 89bb17a into exercism:master May 3, 2019
@cmccandless
Copy link
Contributor

Merged; thanks for working on this!

@simmol simmol deleted the high_score_simplified branch September 6, 2023 10:53
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.

3 participants