Skip to content

leap: Rewrite the test cases and their descriptions #463

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.

Sign up for GitHub

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 1 commit into from
Dec 8, 2016
Merged

leap: Rewrite the test cases and their descriptions #463

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 8 additions & 23 deletions exercises/leap/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,22 @@
{
"cases": [
{
"description": "leap year in twentieth century",
"input": 1996,
"expected": true
},
{
"description": "odd standard year in twentieth century",
"input": 1997,
"expected": false
},
{
"description": "even standard year in twentieth century",
"input": 1998,
"description": "year not divisible by 4: common year",
"input": 2015,
"expected": false
},
{
"description": "standard year in nineteenth century",
"input": 1900,
"expected": false
"description": "year divisible by 4, not divisible by 100: leap year",
Copy link
Contributor

Choose a reason for hiding this comment

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

The "not divisible by 100" restriction is not relevant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, only years that are not divisible by 100 are leap years, aren't they? So IMO "year divisible by 4" alone is not enough, we should also state that the exception does not apply.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only rule that applies here is 'year divisible by 4' you do not need any other rules to determine if 2016 is a leap year.

The 100 rule is only relevant when we have a year that is divisible by 100, which 2016 is not.

Why do you not also reference the 400 year rule here?

What if there were more rules? ("only in evenly numbered decades, unless the decade is 1") Would you include them all in the description?

Copy link
Contributor Author

@guntbert guntbert Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I still disagree - we have a decision tree, where the next rule ("unless") leads to an exemption of the current rule.

So

  • rule 1 fails => common year: only one rule to check, so only one rule in the description
  • rule 1 succeeds => possibly a leap year, we need to check rule 2
    • rule 2 fails: no exemption, so the result of rule 1 holds => leap year, no need to check further roles, so rule 1 and rule 2 in the description
    • rule 2 succeeds => exemption, possibly a common year, we need to check rule 3
      • rule 3 fails: no exemption, the result of rule 2 holds => common year, I mention the two really relevant rules (divisibility by 4 is obvious from mathematics if the number is divisible by 100)
      • rule 3 succeeds: exemption =>leap year only the last rule is really relevant (divisibility by 4 and by 100 is (again) obvious from mathematics.

I really appreciate how you force me to rethink the issue :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that decision tree was very helpful for me to be able to see the way you're approaching the problem, and I can now see how the test descriptions you've used make sense in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also appreciate the effort you've put into this and hope my differing opinions have not discouraged you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries - its good to make well thought decisions - especially when aiming at relatively inexperienced people - and in the end many future tests will draw from the templates in x-common.
How do I proceed from now on? I intend to squash all my commits, thereby amending the commit description. Should I wait on @stevejb71 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do the squash and commit message changes and then @stevejb71 can review the "final" version.

Copy link
Member

Choose a reason for hiding this comment

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

@guntbert from this perspect, you will want to do a squash locally, and then force push your branch, which will set this up like you want it to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

This now looks fine to me.

"input": 2016,
"expected": true
},
{
"description": "standard year in eighteenth century",
"input": 1800,
"description": "year divisible by 100, not divisible by 400: common year",
"input": 2100,
"expected": false
},
{
"description": "leap year twenty four hundred",
"input": 2400,
"expected": true
},
{
"description": "leap year two thousand",
"description": "year divisible by 400: leap year",
"input": 2000,
"expected": true
}
Expand Down