Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"etl",
"trinary",
"beer-song",
"list-ops",
"bowling",
"space-age",
"anagram",
Expand Down
67 changes: 67 additions & 0 deletions exercises/list-ops/example.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
class ListOps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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. :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

VERSION = 1
## Do not use the functions map, concat, count, length, etc.
## and other built in ruby methods to solve these
##
def self.arrays(arr)
counter = 0
arr.each do
counter += 1
end
counter
end

def self.reverser(arr)
answer = []
until arr.empty?
answer << arr.pop
end
answer
end

def self.concatter(arr1, arr2)
answer = []
arr1.each do |element|
answer << element
end
arr2.each do |element|
answer << element
end
answer
end

def self.mapper(arr)
answer = []
arr.each do |element|
answer << (element + 1)
end
answer
end

def self.filterer(arr)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think methods like this could receive a block without relying on a fixed condition like element % 2 == 1. It would be something like self.filter(arr, &block). With this you can call your block inside the each method with block.call(element).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is definitely a possible way to do this, but this seemed very much like a beginner lab and we were trying to use very simple and explicit answers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a problem indeed, but the way filterer was implemented is not exactly like filter, right? I think we could implement simpler methods (the ones that don't expect a block) if this is a problem for beginners. So length, reverse, concat, pop are all good methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The markdown said not to use those methods. Our original example had used all 4 of those, but we felt we needed to re-write all of them to fit what was wanted better.
If I was supposed to use those methods, then it was because the markdown file wasn't very clear since it was only 1 sentence.

answer = []
arr.each do |element|
if element % 2 == 1
answer << element
end
end
answer
end

def self.sum_reducer(arr)
total = 0
arr.each do |element|
total += element
end
total
end

def self.factorial_reducer(arr)
total = 1
arr.each do |element|
total = total * element
end
total
end

end
107 changes: 107 additions & 0 deletions exercises/list-ops/list_ops_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
gem 'minitest', '>= 5.0.0'
require 'minitest/autorun'
require_relative 'list_ops'

class ListOpsTest < Minitest::Test
def test_count_empty
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, people trying to solve this exercise will not be able to see the example.rb file, so the tests should be the guide here. So maybe we could call this test_count_empty_array_elements? This way people know exactly what is expected just by reading the title.

assert_equal 0, ListOps.arrays([])
end

def test_count_normal
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think test_count_normal is not expressive enough in this situation, what is normal in this context? Should we call it test_count_elements?

skip
assert_equal 5, ListOps.arrays(Array.new(5))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

end

def test_count_gigantic
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this test is not adding/testing a new functionality, maybe it can be removed? Was this a problem during implementation?

skip
assert_equal 1_000_000, ListOps.arrays(Array.new(1_000_000))
end

def test_reverse_empty
skip
assert_equal [], ListOps.reverser([])
end

def test_reverse_normal
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we have the same situation here regarding the word normal, what is it in this context? There are a few tests like this, maybe we could change them so the title becomes more expressive?

skip
assert_equal [5, 4, 3, 2, 1], ListOps.reverser([1, 2, 3, 4, 5])
end

def test_reverse_gigantic
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So are we testing for performance with these gigantic tests? Maybe we could remove them if they are not adding new functionality.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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!

skip
assert (1_000_000..1).to_a, ListOps.reverser((1..1_000_000).to_a)
end

def test_concat_empty
skip
assert [], ListOps.concatter([], [])
end

def test_concat_normal
skip
assert [12, 34, 56, 78], ListOps.concatter([12, 34], [56, 78])
end

def test_concat_gigantic
skip
input1 = (1..1_000_000).to_a
input2 = (1_000_001..2_000_000).to_a
assert (1..2_000_000).to_a, ListOps.concatter(input1, input2)
end

def test_mapper_empty
skip
assert [], ListOps.mapper([])
end

def test_mapper_normal
skip
assert [2, 3, 4, 5, 6], ListOps.mapper([1, 2, 3, 4, 5])
end

def test_mapper_gigantic
skip
assert (2..1_000_001).to_a, ListOps.mapper((1..1_000_000).to_a)
end

def test_filterer_empty
skip
assert [], ListOps.filterer([])
end

def test_filterer_normal
skip
assert [1, 3, 5, 7, 9], ListOps.filterer([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
end

def test_filterer_gigantic
skip
assert (1..10_000).to_a.select(&:even?), ListOps.filterer((1..10_000).to_a)
end

def test_sum_reducer_empty
skip
assert_equal 0, ListOps.sum_reducer([])
end

def test_sum_reducer_normal
skip
assert_equal 55, ListOps.sum_reducer([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
end

def test_factorial_reducer_empty
skip
assert_equal 1, ListOps.factorial_reducer([])
end

def test_factorial_reducer_normal
skip
input = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
assert_equal 3_628_800, ListOps.factorial_reducer(input)
end

def test_bookkeeping
skip
assert_equal 1, ListOps::VERSION
end
end