Skip to content

luhn: update test files#479

Merged
robphoenix merged 1 commit intoexercism:masterfrom
robphoenix:luhn/update-tests
Mar 6, 2017
Merged

luhn: update test files#479
robphoenix merged 1 commit intoexercism:masterfrom
robphoenix:luhn/update-tests

Conversation

@robphoenix
Copy link
Copy Markdown
Contributor

@robphoenix robphoenix commented Feb 1, 2017

Add an example_gen.go testCases generator to generate the testCases from
the x-common canonical data. Generate the new testCases data and update
the example to pass the new test data. Bump the testVersion to 2.

@robphoenix
Copy link
Copy Markdown
Contributor Author

I realise exercism/problem-specifications#522 and exercism/problem-specifications#523 are still open and could well affect this PR, so our track should leave this unmerged until they are resolved, and any relevant updates can be made to these commits.

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Sure, we can look at this again when the two x-common issues are done.

The circumstance in which we would merge it now:

  • We predict that neither x-common issue will require a change to the generator or example solution; we just run the generator again.
  • We think there is a big advantage in getting the tests in this PR out to the students out -- they offer something the current tests don't (currently the distinction is pretty small, but you could convince me otherwise)

Alternatively if you just want to pull the "move TestTestVersion" into a separate PR we can get that merged now; it doesn't depend on having a generator.

Also, probably good to prefix the "move TestTestVersion" commit with "luhn:"

@robphoenix
Copy link
Copy Markdown
Contributor Author

separated into 2 PRs (#480)

@robphoenix
Copy link
Copy Markdown
Contributor Author

I've updated this to reflect the merged changes form exercism/problem-specifications#522, the example.go has changed again so do make sure you're happy with it before approving.
Still waiting on exercism/problem-specifications#523 before merging

Comment thread exercises/luhn/example.go Outdated
d := make([]int, 0, len(id))

for _, r := range id {
if r != ' ' && (r < '0' || r > '9') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

personally, I would strive to avoid the repetition of the '0' and '9', by saying continue if r == ' ', then the remaining cases are if/else of each other. Assuming I didn't get my logic wrong. Does it make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that makes better sense, yes

Comment thread exercises/luhn/example.go Outdated
return d

last := len(d) - 1
return (check(d[:last])+d[last])%10 == 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm assuming check returns the sum? Would anything go wrong if we just used check(d) % 10 == 0?

E: Oh never mind, this is a same line as above, just moved. I guess we don't need to change it, though if it's going to show up in git diff as a diff anyway, maybe we could consider

@robphoenix
Copy link
Copy Markdown
Contributor Author

updated to take into account @petertseng comments. The sum function (formerly check function) is more deeply nested than I would like but I think makes things a little clearer now that the Valid function can return check(d)%10 == 0 rather than what it did before.

Comment thread exercises/luhn/example.go Outdated
}
if r >= '0' && r <= '9' {
d = append(d, int(r-'0'))
} else {
Copy link
Copy Markdown
Member

@petertseng petertseng Feb 14, 2017

Choose a reason for hiding this comment

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

this makes me realize there are some alternatives for how to structure this. They all have some advantage or disadvantage, so you can pick one you like after trying out:

  1. Don't bother having if r == ' ' { continue } above. Instead just have else if r != ' ' { return false } here. Pro: Fewer clauses. Cons: May be a little harder to understand in what cases it does neither.

  2. Invert the checks, say if r < '0' || r > '9' { return false } above. Pro: Now the d = append can be unindented. Con: Have to read through two conditions before getting to the "actual" loop body (the case that causes something interesting to happen)

  3. Leave as-is. Pro: Smaller diff against current code. Con: typical-case loop body sandwiched between two other cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I chose door no. 2 👍

@robphoenix
Copy link
Copy Markdown
Contributor Author

robphoenix commented Feb 14, 2017

@robphoenix
Copy link
Copy Markdown
Contributor Author

I think we can merge this now without waiting on exercism/problem-specifications#523 to be resolved, we can update the exercise when that happens.
Unless there's any objections I think I'll merge this in the next day or so.

Add an example_gen.go testCases generator to generate the testCases from
the x-common canonical data. Generate the new testCases data and update
the example to pass the new test data. Bump the testVersion to
2.
Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I find it likely that exercism/problem-specifications#523 will not change the structure of the JSON file, so we can simply rerun the generator at that time, and not wait.

Of course, there is a new JSON schema, but when that gets applied it might make our lives easier since supposedly that means each exercise's JSON file will have in common, which means we can write less exercise-specific code. We will see if that bears fruit!

@robphoenix
Copy link
Copy Markdown
Contributor Author

👍

@robphoenix robphoenix merged commit d5ed611 into exercism:master Mar 6, 2017
@robphoenix robphoenix deleted the luhn/update-tests branch March 6, 2017 10:03
@petertseng petertseng removed the on hold label Mar 6, 2017
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.

2 participants