Update/sum of multiples#427
Conversation
default values are no longer mentioned in the README, so this note is no longer necessary. exercism#256
As there is no longer any consideration for default values having a separate test for 3 and 5 as divisors is unnecessary and could potentially confuse.
fixes exercism#340 Instead of returning a function, MultipleSummer() now returns an integer. This change acknowledges that while higher-order functions have a valuable place, the higher-order function here was rather shoe-horned in and made the exercise slightly confusing.
|
I've left the example function as variadic. I quite like it, and like the original exercise implementer in #53 think it could be a good opportunity to introduce variadic functions. Though I do also realise this goes against the recommendation from @petertseng to just take a slice instead, and could very well be considered inappropriate. I'm happy to change to a slice if preferred. |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
These changes are all welcome, and some are way overdue! Thanks for putting in the work.
Before merging, I have one question about whether the function should be renamed to something other than MultipleSummer, take a look.
I've left the example function as variadic.
Though I do also realise this goes against the recommendation
I should really reconsider that part of the recommendation. Of the two changes:
- Make the function takes all of its parameters at once, rather than a function that returns a function [1]
- slice vs variadic
The former is definitely the much more important part of the recommendation. In fact, I may have made the slice suggestion without truly thinking about it. I am happy to keep it variadic, and if someone really wants to use a slice, they can make another PR arguing why that should be.
[1] Aside for those who are interested in studying languages: See languages that support partial application by making all their functions take only one argument at a time: Haskell and OCaml both do this by default, and Scala has special syntax for multiple argument lists.
| } | ||
| // MultipleSummer returns the sum of the multiples of the given divisors | ||
| // up to, but not including, the given limit. | ||
| func MultipleSummer(limit int, divisors ...int) (sum int) { |
There was a problem hiding this comment.
it looks like historically it made sense to call this MultipleSummer because it would be used like MultipleSummer(3, 5).sum(4) or something.
At this point, now it is being used like MultipleSummer(4, 3, 5), in which case I think it is better to give it a verb name than a noun name. I am thinking of SumMultiples. go lint may complain about summultiples.SumMultiples being redundant, but maybe someone else has a better suggestion.
What say you? I can't find justification for this in either https://github.com/golang/go/wiki/CodeReviewComments or https://golang.org/doc/effective_go.html so it may be a personal thing, but I usually see function names as verbs.
There was a problem hiding this comment.
Yeah, summultiples.SumMultiples isn't perfect but will do for now, no complaints from go lint.
| } | ||
| } | ||
|
|
||
| func TestVar(t *testing.T) { |
There was a problem hiding this comment.
With the removal of Test35 we could rename this, since the Var is no longer needed to distinguish it from the 35. I'm not sure that simply Test will be accepted, but maybe TestSum will suffice. Optional change.
| } | ||
| } | ||
|
|
||
| func BenchmarkVar(b *testing.B) { |
There was a problem hiding this comment.
same idea, maybe this can get renamed now.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
Fantastic. Let's take it.
In case anyone wonders about test version... remember that its purpose is to let reviewers know what version of the tests a given submission is written against. In that light, it may not be necessary to bump it here, since a reviewer might be able to tell by seeing the difference in function name.
If someone thinks it is very important to let reviewers see this at a single glance at one line (it is certainly the case that not all reviewers follow the developments in this repository, so it can aid them), we can add them in.
|
oh gosh yeah, I totally overlooked the test version, sorry! |
2 participants
No description provided.