add list-ops#332
Conversation
rhoprhh
force-pushed
the
list-ops
branch
3 times, most recently
from
00ea02a to
a7a5901
Compare
|
ready for review |
|
@InvasiveLionfish collaborated with me on building these tests |
| @@ -0,0 +1,67 @@ | |||
| class ListOps | |||
There was a problem hiding this comment.
Hi @rhoprhh! Since you are adding these methods in your own class it is ok to have methods named map and concat inside it. Any motive to use your own names?
There was a problem hiding this comment.
The markdown document said to not use existing functions. I just wanted to make sure it was very obvious that you were writing your own methods and not just using array enumerators to do it.
Here is the entire .md document:
"In functional languages list operations like length, map, and reduce are very common. Implement a series of basic list operations, without using existing functions."
There was a problem hiding this comment.
Since this file is an example, I don't think using the "real" names is a problem. But no strong feelings about this, just my opinion. :)
There was a problem hiding this comment.
I lean slightly towards the direction of preferring the real names, because people already know what they do. It's obvious when looking at the code that these aren't just using the standard library implementations, so I think that would be totally fine.
|
I will review this in a few hours, looks like it is at that stage. Let me know if it should not yet have the 'ready' label on it. |
|
|
||
| def test_count_normal | ||
| skip | ||
| assert_equal 5, ListOps.arrays(Array.new(5)) |
There was a problem hiding this comment.
This method name is a little misleading since I'm expecting multiple arrays instead of elements.
So maybe we could call it elements instead, or even length?
|
The overall test time on my (admittedly slow) computer is 5.77 seconds. On a one time run. Is this normal, or is this because of the 'gigantic' tests? |
| assert_equal [5, 4, 3, 2, 1], ListOps.reverser([1, 2, 3, 4, 5]) | ||
| end | ||
|
|
||
| def test_reverse_gigantic |
There was a problem hiding this comment.
So are we testing for performance with these gigantic tests? Maybe we could remove them if they are not adding new functionality.
There was a problem hiding this comment.
Agreed. In general here's my philosophy with respect to test data:
- as small as possible
- as boring as possible
If I'm testing "more than one", then I use 2, unless there's something about a longer list that will potentially have different behavior that could be missed with two (a middle element, some sort of overlap, etc).
I want the test data to express as succinctly as possible what we are actually testing, so I avoid "real-ish" data, because then the person is left guessing why that data was used. Incidental features in the "real-ish" data can act as red herrings, and people make up mental models of what the problem is about that might not be accurate, and that are definitely not necessary.
There was a problem hiding this comment.
@rhoprhh Just checking in to see if there are any changes coming in from your side... Thanks for all the work so far! Looking forward to getting these in!
|
Has there been any progress on this? If not I suggest we should just merge it and have a less-than-perfect problem out there for people to try and then suggest improvements to. |
Yeah, let's do it. |
nth-prime: Add JSON test data
After exercism#332 was created but before it was merged, exercism#351 was created and merged moving all exercises to a new structure. The nth-prime JSON file should now be moved to its place in the new structure as well.
5 participants
No description provided.