-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
custom-set: Stub, order of methods, rename some methods #286
Conversation
The current `empty` method is mutable and removes all the elements of a set. This method is rather pointless, since one could simply refer to `CustomSet->new` and throw away the old set. The updated `is_empty` returns whether a set is empty, which is equivalent to testing that the size is zero, but abstractly different. Compare this to the Haskell track's `empty` which is the empty set, and `null` which is the predicate function. These are Haskell conventions; `CustomSet->new` is an adequate way to create an empty set in Perl. Compare this to the Ruby track's `empty` which is the empty set, and `empty?` which is the predicate function; this is a Ruby convention. It seems that since we already have a canonical way to represent the empty set, the only thing we need is a predicate function and a name that suggests that it is a predicate function.
This avoids overlap with the built-in `delete`, which confuses syntax highlighters and potentially students. If there is a learning goal in dealing with method names that collide with built-in functions, this seems unintentional.
To make it more clear what exercises are to be implemented, and what arguments they take, this exercise stub lists the methods with their expected arguments and dies with a standard error message. The constructor is listed first. Then the two mutable methods `add` and `remove` to distinguish them. Then the simplest immutable methods, so `is_empty`, `is_member`, `size` and `to_list`. Then `union`, `intersect` and `difference` are listed: If written first they can be used to define the remaining functions, so encourage the student to write them first. Listed in order of simplicity. Lastly `is_disjoint`, `is_subset` and `is_equal` are listed as the most difficult, made simpler if the previous methods were already written. Also listed in order of simplicity.
Should we avoid features like subroutine signatures in example solutions because they don't exist in Perl 5.18 and below? I've intentionally omitted them from the stub to stay backwards compatible, but included them in the example solution because I consider them idiomatic within some style of modern Perl. Perhaps examples should only be tested with later versions of Perl, since the student is not required to seek them out. |
Update the example solution so that it uses subroutine signatures. Since they were added in Perl 5.20, use 5.020. Declare what is being exported. Re-implement some methods using set methods. Propagate the method order in a32b51f2ecead5c8730f5e3967538af3bcf105d7 to the example solution and the unit tests.
I would say it's best to avoid them in the examples, as I figure the primary intention of the examples is to ensure that the tests work properly across all versions. Signatures are nice, but are seemingly rarely used in Perl 5. Maybe it'd be worth adding documentation with a link to a list of perl features (e.g. https://perldoc.pl/feature) for users to explore? |
OK. I'll remove them from the example.
We use them at work all the time; that is, old code still hasn't got them, but when I refactor old subroutines that appear to be called in predictable ways, I always add them as a measure of safety, since signatures will error on the wrong number of arguments, which is a weak kind of type-safety. |
Remove `use Exporter 'import';` as all subs are class methods.
Remove `use Exporter 'import';` in solution, too, as all subs are class methods.
I've refrained from adding generator support (#249). That issue says you'd like to migrate tests to Test2 first. I'd consider both out of the scope of this PR. Let me know if the PR is missing anything. |
Pretty much looks good to me, just need to take out the |
OK, didn't get that. They're removed now. |
For this PR I haven't written a test generator yet, as it was recommended in #241.