-
-
Notifications
You must be signed in to change notification settings - Fork 531
Add hash concept and exercise #1809
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
base: main
Are you sure you want to change the base?
Conversation
c4876ae to
d89a432
Compare
|
I would say we are a bit in a limbo, though, the array and enumeration concepts are connected to one exercise, and personally, I think it makes sense to introduce arrays before hashes. But the enumeration concept mentions hashes. So either I think enumeration should be stripped out of hashes, which might not make sense. Instead it might make sense to make a separate exercise for arrays so we can have. |
|
An option could be to temporairly place hashes over arrays |
I do tend to teach "collections" rather than independent data structures, since other than the data structure itself, the idea of Enumerable being the thing that gives a lot of power. What do you think about taking that approach for a concept, rather than the concept of "map/dictionary" specifically? I have not looked at the changes in this PR yet, though. Just got back to my computer after a two week break (travelling). |
|
|
||
| Even though hashes are unordered collections, Ruby maintains the insertion order of key-value pairs. | ||
| This means that when you iterate over a hash, the pairs will be returned in the order they were added. | ||
| However, deleting elements may affect the order of remaining elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand on this, is the order at that point of operation undeterminate? Citation would be helpful for further exploration, and historically in Ruby, this is an interesting point as well.
concepts/hashes/about.md
Outdated
| my_hash = { 1 => "one", :two => 2, "three" => [3, "three"] } | ||
| ``` | ||
|
|
||
| Alternatively if the keys are symbols, you can use a more JSON-style syntax: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps avoid JSON reference. But maybe mention the "newer syntax as of Ruby 1.9" form as shown.
concepts/hashes/about.md
Outdated
| my_hash = { name: "Alice", age: 30, city: "New York" } | ||
| ``` | ||
|
|
||
| You can create an empty hash using the `Hash.new` method: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize "Hash" when speaking of the type specifically or the object itself, since hash is also a method and an idea. (Even if it weren't this situation, I would likely suggest capitalization as well as code syntax for the specific thing.)
concepts/hashes/about.md
Outdated
|
|
||
| ## Accessing values | ||
|
|
||
| You can access values in a hash using their corresponding keys, the syntax reminds of array indexing, but using the key instead of an index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| You can access values in a hash using their corresponding keys, the syntax reminds of array indexing, but using the key instead of an index: | |
| You can access values in a `Hash` instance using its corresponding keys, the syntax reminds of array indexing, but using the key instead of an index: |
concepts/hashes/about.md
Outdated
| You can access values in a hash using their corresponding keys, the syntax reminds of array indexing, but using the key instead of an index: | ||
|
|
||
| ```ruby | ||
| my_hash = { "name" => "Alice", "age" => 30, "city" => "New York" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No space inside Hash delimiters, similar to Array and Range, save the space inside {} delimiters for blocks.
| my_hash = { "name" => "Alice", "age" => 30, "city" => "New York" } | |
| my_hash = {"name" => "Alice", "age" => 30, "city" => "New York"} |
Alternatively, present it as:
| my_hash = { "name" => "Alice", "age" => 30, "city" => "New York" } | |
| my_hash = { | |
| "name" => "Alice", | |
| "age" => 30, | |
| "city" => "New York" | |
| } |
Indentation adjusted for readability (this is how I style my key/value pairs reducing eye jitter), and not necessary, but might be nice to have.
| @@ -0,0 +1,38 @@ | |||
| class GrossStore | |||
| UNITS = {'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144, 'great_gross' => 1728}.freeze | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that there is no space inside the delimiter, but why the freeze? We advertise this as a mutable object, but then make it immutable here?
Also blank line after class definition line is a signal of "topic change" and creates a paragraph of sorts.
This is an exemplar, not just an example, so we should apply opinions here to be that exemplar.
But I will not critique explicitly here. Welcome to discuss it on Discord, though, if you want to (even pair program through it if you want).
Or even as a product of a mentor request! ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here was mostly that a user shouldn't modify that hash since it is supposed to be a constant throughout the exercise. So if a user tried to modify it while working with it, it would raise an error.
| @@ -0,0 +1,24 @@ | |||
| class GrossStore | |||
| UNITS = { 'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| UNITS = { 'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144, | |
| UNITS = {'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144, |
As a minimum, no space inside delimiters.
Perhaps a columnar presentation of the Hash as well, easier to see patterns, etc., than having a horizontal showing of this.
| UNITS = { 'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144, | ||
| 'great_gross' => 1728 }.freeze | ||
|
|
||
| attr_reader :bill |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public method definition above the private method initialize is something that I personally detest, it suggests something that is untrue (that initialize is perhaps not private, since in this organization it is in the midst of public method definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most online resources I found is attr_reader placed above initialize.
| ], | ||
| "prerequisites": [ | ||
| "advanced-enumeration" | ||
| "hashes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be capitalized, as hashes are created by using hash method calls, and is something different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the slug
| ], | ||
| "prerequisites": [ | ||
| "ostruct" | ||
| "hashes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... same here.
I think in a way that is a good idea, I think however that the concept might end up being too big. Since this concept is around 200 lines say array is the same and then what about other collections (such as sets). It might make sense to have these after each other but don't know about having a single big one. |
I would not likely cover sets, but I teach about strings, as a collection as well. Later (intermediate) I teach "what would happen if..." in order to teach mixins with Enumerable. But yes, no need to teach all the collections, Hash and Range and Array being more than sufficient to gain insights into collections in general, and perhaps not going so far as to "how to create your own 'collection' class". |
|
On 26/01/08 01:24PM, meatball wrote:
@meatball133 commented on this pull request.
> +
+Implement the `add_item` method, which takes two parameters: `item` (String) and `unit` (String).
+
+```ruby
+gross_store = GrossStore.new()
+gross_store.add_item("apple", "dozen")
+# => true (since dozen is a valid unit)
+gross_store.bill
+# => { "apple" => 12 }
+```
+
+## 3. Remove an item from the customer bill
+
+To implement this, you'll need to:
+
+- Return `false` if the given item is **not** in the bill
Answer to both the question about explicitly return `true` and this one. So no return would be a possibility for this method but Ruby doesnt really have a specific no return instead, that is nil. And then we would have double nil for both truthy and falsey. What I think could make sense for the delete method could be that it returns the quantity before the operation, and nil if it failied.
But otherwise I think it doesnt make sense that it returns true and then nil if it fails, that to me just seems weird
It does make sense from a boolean perspective, but not from a Boolean
perspective, if you get my subtle communication there. Falsey/truthy values are
something common for Rubyists. We do not have to necessarily enforce explicit
`true` or `false` returns everywhere. Leave that portion of the solution up to
the student to decide how they want to do this? In other words, assert or
refute, but not assert equals true or assert equals false.
Keeping it a little loose for the non-concept specific things are fine, while
the concept portion is often a lot more strict than what a practice exercise
would be.
|
d89a432 to
aced6ca
Compare
I just checked the code and there is nowhere we actually do On another note, rubocop complains when there aren't spaces around the start and end of a hash, I think that was the reason I added them. So I think we would have to disable that if we don't want it |
|
On 26/01/09 03:31AM, meatball wrote:
@meatball133 commented on this pull request.
> @@ -0,0 +1,38 @@
+class GrossStore
+ UNITS = {'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12,
'small_gross' => 120, 'gross' => 144, 'great_gross' => 1728}.freeze
They keys are already immutable (in effect as a minimum), since keys are compared
using the hash value of the key, rather than the "content" or "value" of the key
itself, and integers are immutable as well, so protecting the reassignment of
the value, right?
I am not sure if this is an important part of the concept of Hash, since it is
not generally immutable, and so we are teaching outside of the concept a bit by
freezing it.
The idea here was mostly that a user shouldn't modify that hash since it is
supposed to be a constant throughout the exercise. So if a user tried to
modify it while working with it, it would raise an error.
Constants are not generally frozen in Ruby, (modules and classes are
constants) and "constant" in Ruby means "constant of type" not "constant of
value" nor "constant of behavior".
I do not (yet) quite see the benefit of freezing this Hash as I do not see it
tying into the concept of `Hash`.
We could communicate this via tests, rather than raising an exception from the
`freeze`, which in my opinion is better. When the student violates the concept
of Hash, then the tests should report that, if we are teaching the concept of
Hash.
Does that make sense?
|
|
On 26/01/09 03:38AM, meatball wrote:
@meatball133 commented on this pull request.
> @@ -0,0 +1,24 @@
+class GrossStore
+ UNITS = { 'quarter_of_a_dozen' => 3, 'half_of_a_dozen' => 6, 'dozen' => 12, 'small_gross' => 120, 'gross' => 144,
+ 'great_gross' => 1728 }.freeze
+
+ attr_reader :bill
In most online resources I found is attr_reader placed above initialize.
Yeah, I know... and they are "wrong". ;) And as we know "most often done" does
not necessarily mean "the right thing to do", nor even the "best way to do it".
If we consider the access and scope of what this does, having the "jitter" is
not really desired.
Even the style guide infers that `initialize` is public because it states "above
other public methods" even though `initialize` is not public. Having a private
method nested in the midst of public methods at a minimum infers that
`initialize` is something other than private. And that is objectively not true.
Also, a reason why `initialize` is often not placed after a `private` method
call is that Yardoc breaks behavior that it's core tool (rdoc used internally)
does not break, and that is the documentation of the public constructor `new`.
I think that is a bug in Yardoc, but have not been motivated enough to submit a
fix for it, I likely have also not reported it, or check that it has been
reported.
|
|
On 26/01/09 04:10AM, meatball wrote:
meatball133 left a comment (exercism/ruby#1809)
> In other words, assert or
refute, but not assert equals true or assert equals false.
I just checked the code and there is nowhere we actually do `assert_equal
true` or false for that matter. We use `refute` and `assert`. But I think it
might confuse the student if we say return a truthy or falsey value, I think
it might be better with `true` and `false`.
I think that we can say "true" but not `true` there and it would be fine.
Hopefully the students will take that as meaning that we are not necessarily
enforcing true, but also mentoring can bring that out.
On another note, rubocop complains when there aren't spaces around the start
and end of a hash, I think that was the reason I added them. So I think we
would have to disable that if we don't want it
Sounds like a potential addition to the `rubocop.yml` configuration file.
|
This is apart of making ostruct an optional concept: https://forum.exercism.org/t/openstruct-is-now-officially-discouraged/17490