-
-
Notifications
You must be signed in to change notification settings - Fork 365
Added Largest Series Product Generator #474
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
Added Largest Series Product Generator #474
Conversation
Created Generator for DifferenceOfSquares (exercism#471)
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.
Some small changes requested.
@@ -22,6 +36,7 @@ private static long ParseDigit(char c) | |||
{ | |||
if (!char.IsDigit(c)) | |||
{ | |||
Console.WriteLine("Invalid digit: " + c); |
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.
Could you remove this line?
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.
Yes!
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.
I reverted all of my changes to Example.cs
:)
[Fact(Skip = "Remove to run test")] | ||
public void Rejects_empty_string_and_nonzero_span() | ||
{ | ||
Assert.Equal(-1, LargestSeriesProduct.GetLargestProduct("", 1)); |
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 -1
is an error case, and should throw an exception. Same goes for the other -1
cases.
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.
So, if I'm understanding this, a -1
in the canonical-data.json
represents an exception?
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.
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.
I added the Assert.Throws
!
protected override void UpdateCanonicalData(CanonicalData canonicalData) | ||
{ | ||
foreach (var canonicalDataCase in canonicalData.Cases) | ||
canonicalDataCase.Property = "GetLargestProduct"; |
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.
Maybe CalculateLargestSeriesProduct
would be a better name.
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.
Hmm. Maybe?
I interpret GetLargestProduct
as I wanna get the largest product in this series, which fits the exercise description. Basically the same as saying CalculateLargestSeriesProduct
, but more precise. I would leave it, but regardless, both convey the correct meaning.
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.
Well argued. I agree
- Changed name of Property to GetLargestProduct, to fit with example and stub implementation - Added Assert.Throws for scenarios where `canonical-data.json` expected a failure (described as `-1`) - Updated eol to canonical (unintended, but the generator created it).
e43e539
to
ca8fd8d
Compare
I think I've fixed all of the issues. Thanks for the feedback! |
Thanks! |
⛵ :D |
GetLargestProduct
, to fit with example and stub implementationeol
to canonical (unintended, but the generator created it).