No longer drives the solution #170

kotp · 2015-06-16T04:59:12Z

Does not rely on on any specifically named 'helper' methods. Tests
things through the implementation via public facing API

re: #10

I removed the test for "identity" as well, as there was nothing that indicated that a empty string should return 1, which is what the test stated.

The example file was changed against the original specification provided by the README and the tests prior to these changes.

Does not rely on on any specifically named 'helper' methods. Tests things through the implementation via public facing API re: #10

kytrinyx · 2015-06-16T05:45:45Z

largest-series-product/largest_series_product_test.rb

-    expected = [[9, 8, 2], [8, 2, 3], [2, 3, 4], [3, 4, 7]]
-    assert_equal expected, series.slices(3)
+  def test_required_method_called_largest_product
+    assert_respond_to Series.allocate, :largest_product


What does this test mean?

It is the one test that specifies the signature that Series will have an instance method called largest_product. The signature of the test itself is object, method_name, message.

(from gem minitest-5.7.0) === Implementation from Minitest::Assertions ------------------------------------------------------------------------------ assert_respond_to(obj, meth, msg = nil) ------------------------------------------------------------------------------ Fails unless obj responds to meth.

The documentation via RDoc demonstrates it well.

==== (Uses superclass method RDoc::AnyMethod: MiniTest::Assertions#assert_respond_to) ------------------------------------------------------------------------------ Tests if the given Object responds to method. An optional failure message may be provided as the final argument. assert_respond_to("hello", :reverse) #Succeeds assert_respond_to("hello", :does_not_exist) #Fails

I could have allocated the object separately, and used a variable, but it is succinct enough here, or I could have instantiated it, knowing the signature we want, but it would then fail with an argument error for initialize, and I wasn't sure we wanted to start driving the initialize method here. Though of course we could. If the important thing is the method itself, better to have it only address exactly what we want to address.

It is perhaps not the first test that should be ran, though. Perhaps it should be the tests that would drive the creation of the initialize method.

Thanks for walking me through that!

There's another test that makes an assertion about the result of calling largest_product on the instance. Do we need this test in addition to the other one?

I think it depends on the purpose of the test. If it is to get someone started with prerequisites, with that being the name of the method itself, this addresses that. If it is an aside due to testing the method outright, then it accomplishes the same thing, by side-effect.

It really goes to how much the tests help drive the specification and communication, more than duplication.

Line 67, for example, accomplishes the same thing, in effect, but concentrates that the method operates as expected, not that it exists. Of course, if it doesn't exist, it will also drive that eventuality.

Cleaner, perhaps, to state that requirement of existence before how it will operate. For sure, in a TDD manner, it would be appropriate to have some test state that a method exists, before testing for functionality.

Of course, I am up for suggestions.

I have other changes to make as well, just waiting for feedback on this, then I can make the update. So this isn't quite ready to go yet... I will commit to this branch other changes that are pending currently, as well as any test changes you see fit.

It hadn't ever occurred to me to specify the API of a library explicitly, separately from the behavior. I'm still not convinced about it, but I'm open to the possibility that there's some benefit that I'm missing.

At this point I'd rather keep the tests about the behavior, with the API specified as a side effect.

I will make the change. Thanks.

As per discussion in PR #170 re: #10 This tests the existence of `largest_product` via error rather than failure. Provides the small numbers as the first two tests.

kytrinyx · 2015-06-17T05:35:51Z

I removed the test for "identity" as well, as there was nothing that indicated that a empty string should return 1, which is what the test stated.

I think that the largest product of nothing should be the identity. I'm not sure why you removed it. Because this isn't in the README?

kotp · 2015-06-17T05:50:41Z

Yeah, that was the only reason, there was nothing to indicate what it should be other than the test. The largest product of nothing should be 0 perhaps? I don't know what the set theory is, or if it applies, or just the arithmetic theory.

In fact, I thought given a null set of digits, it would possibly be invalid and should raise an error. But not sure of the theory behind it.

markijbema · 2015-06-17T06:37:03Z

largest-series-product/example.rb

  end

-  private
+  def analyzer


little naming nitpick: i think it's clearer when a method which returns something is named after the thing it returns, so maybe 'maximum' or if you want to show that the analysis is the important thing here I would say something like analysis_result. When you have a method called analyzer I would expect it to return a lambda (or object responding to call)

Agreed, naming is hard. But analyzer_result is a duplication, as result is assumed, probably, as it is the return of the method.

Maybe largest_value or even _largest_product to avoid the duplication of the method name itself?

kotp · 2015-06-17T06:40:14Z

I was considering adding an actual message to clarify, what do you think @markijbema ??

markijbema · 2015-06-17T06:40:43Z

I like this way better. One thing which immediately strikes me is that I would implement this differently now. I would just sort the digits, take the last few, and multiply them. A solution of which I didn't think before (at least).

markijbema · 2015-06-17T06:41:17Z

Adding an explicit message would even be better, agreed.

kotp · 2015-06-17T06:41:22Z

Can't sort the digits, as the product group changes when you do. I had initially mis-read the information given, and was doing that.

markijbema · 2015-06-17T06:42:31Z

ah yes, you're right. Forgot the exercise, but it says series of digits. I think given the tests, it would still be my first attempt for the third test ;) I'm really liking this!

kotp · 2015-06-17T06:43:01Z

Pair with me real quick?

markijbema · 2015-06-17T06:44:57Z

Sorry, I'm at work (but always start the day by github notifications of both work and outside of it), so don't have the time. Btw, Katrina is right about the identity, it should be 0. If you're interested, it's indeed group(/field) theory. Over addition, 0 should be the identity, over multiplication it should be 1.

kotp · 2015-06-17T06:49:59Z

It was 1. so I will restore that, but should there be a mention of "why" in the README or a reference somewhere?

re: #10 Changes per discussion at PR: #170

kotp · 2015-06-17T08:07:04Z

Thanks @markijbema for clarifications. Added messages and restored the identity test. Also refactored method I think it is better.

markijbema · 2015-06-17T12:18:40Z

I think this is good to merge. I don't feel to strong either way over the explanation of the identity, but then again, I did mathematics ;)

@kytrinyx also ok with merging this? Do you want explanation of the identity?

kytrinyx · 2015-06-18T03:26:02Z

I think this is good like this. Identity can be a bit of a rabbit hole.

No longer drives the solution

kotp · 2015-06-18T03:50:40Z

I was just getting ready to squash, when I was checking the state of master and was surprised by the traffic coming in. Nice surprise.

Thanks @kytrinyx !

largest-series-product: Comment on tricky cases

No longer drives the solution

ecdf7fb

Does not rely on on any specifically named 'helper' methods. Tests things through the implementation via public facing API re: #10

kytrinyx reviewed Jun 16, 2015
View reviewed changes

Testing via error rather than failure

9176851

As per discussion in PR #170 re: #10 This tests the existence of `largest_product` via error rather than failure. Provides the small numbers as the first two tests.

markijbema reviewed Jun 17, 2015
View reviewed changes

Restore identity, add hint messages

7e52f2d

re: #10 Changes per discussion at PR: #170

kytrinyx added a commit that referenced this pull request Jun 18, 2015

Merge pull request #170 from exercism/10_forces_implementation_concerns

b1dba08

No longer drives the solution

kytrinyx merged commit b1dba08 into master Jun 18, 2015

kytrinyx deleted the 10_forces_implementation_concerns branch June 18, 2015 03:26

gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016

Merge pull request exercism#170 from petertseng/lsp-comments

58e4a59

largest-series-product: Comment on tricky cases

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer drives the solution #170

No longer drives the solution #170

kotp commented Jun 16, 2015

kytrinyx Jun 16, 2015

kotp Jun 16, 2015

kytrinyx Jun 17, 2015

kotp Jun 17, 2015

kytrinyx Jun 17, 2015

kotp Jun 17, 2015

kytrinyx commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema Jun 17, 2015

kotp Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kytrinyx commented Jun 18, 2015

kotp commented Jun 18, 2015

No longer drives the solution #170

No longer drives the solution #170

Conversation

kotp commented Jun 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx commented Jun 17, 2015

kotp commented Jun 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kotp commented Jun 17, 2015

kotp commented Jun 17, 2015

markijbema commented Jun 17, 2015

kytrinyx commented Jun 18, 2015

kotp commented Jun 18, 2015