Skip to content

add PascalsTriangle generator #459

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

Merged
merged 3 commits into from
Oct 5, 2017

Conversation

bmeverett
Copy link
Contributor

Adds the PascalsTriangle generator. Pretty straight forward, except for the exception cases. I modified the Example to get around the yield return so the exception would be thrown. And the final test case, where the value is null, I converted that back to -1 because you can't run a test with the incorrect number of parameters as the solution doesn't build.

Closes #418

dataCases.ExceptionThrown = typeof(ArgumentOutOfRangeException);
if (dataCases.Properties["count"] == null)
dataCases.Properties["count"] = -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be converting this to a -1? It should be entirely possible to pass in null (int?), or pass in nothing with a default param.

Copy link
Member

Choose a reason for hiding this comment

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

@jpreese Well, I think the canonical data is at fault here. In what way would passing null make sense for calculating pascal's triangle? I've created an issue to discuss removing this strange case. In the mean time, I'm in favor of manually removing this error case from the canonical data to make sure we don't output it.

Copy link
Member

Choose a reason for hiding this comment

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

@jpreese @bmeverett I've created a PR to fix the canonical data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd agree. I know there was a PR over in the problem-specs repo about wanting to test edge cases (e.g. null). I'm not sure if this is a result of that or where that got left off.

}

private static IEnumerable<IEnumerable<int>> ItterateRows(int rows)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

ItterateRows should be IterateRows

protected override void UpdateCanonicalData(CanonicalData canonicalData)
{
foreach (var dataCases in canonicalData.Cases)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change dataCases to dataCase (most use canonicalDataCase). Since the variable is a single case.

@jpreese
Copy link
Contributor

jpreese commented Oct 5, 2017

Just some small nits @bmeverett, looks good though!

dataCases.ExceptionThrown = typeof(ArgumentOutOfRangeException);
if (dataCases.Properties["count"] == null)
dataCases.Properties["count"] = -1;
}
Copy link
Member

Choose a reason for hiding this comment

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

@jpreese Well, I think the canonical data is at fault here. In what way would passing null make sense for calculating pascal's triangle? I've created an issue to discuss removing this strange case. In the mean time, I'm in favor of manually removing this error case from the canonical data to make sure we don't output it.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I've found one last issue, and once that is fixed I'll merge :)

@@ -7,4 +7,14 @@ public static IEnumerable<IEnumerable<int>> Calculate(int rows)
{
throw new NotImplementedException();
}

private static IEnumerable<int> Row(int row)
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this function? The stub implementation only needs the public methods to allow it to compile.

throw new NotImplementedException();
}

private static IEnumerable<IEnumerable<int>> IterateRows(int rows)
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this function? The stub implementation only needs the public methods to allow it to compile.

@ErikSchierboom ErikSchierboom merged commit ccb207c into exercism:master Oct 5, 2017
@ErikSchierboom
Copy link
Member

Merged. Thanks a lot! 🎉

@bmeverett bmeverett deleted the 418-pascalsTriangle branch October 5, 2017 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants