Skip to content
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

nucleotide-count: refactoring tests (discussion) #895

Closed
wants to merge 3 commits into from
Closed

nucleotide-count: refactoring tests (discussion) #895

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bd8580
refactored the nucleotide-count test data
Vankog Sep 8, 2017
4eb42d7
refining nucleotide-count test after feedback:
Vankog Sep 15, 2017
b22ce4a
new revision of nucleotide count
Vankog Oct 6, 2017
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
56 changes: 48 additions & 8 deletions exercises/nucleotide-count/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
{
"exercise": "nucleotide-count",
"version": "1.0.0",
"comments": [
"Given a string of nucleotides, the count of each nucleotide is returned.",
"",
"It is the track's responsibility to add further language specific tests like:",
"input error handling (null/undefined parameters etc.)",
"referential transparency (i.e. evaluating, a function/method gives the same value for same arguments every time.)",
"",
"'error' and 'null' should be treated according to the languages' specifics and possibilities."
Copy link
Member

Choose a reason for hiding this comment

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

since there is no more null that I see (I could have missed), the part about null wouldn't be necessary

],
"version": "1.1.0",
"cases": [
{
"description": "count all nucleotides in a strand",
"description": "counts all nucleotides in a provided strand.",
"comments": [
"nucleotideCounts counts all DNA-nucleotides in the input strand and returns an object with properties",
"A, C, G and T holding their respective amounts."
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the longer description into a comment like others do it.

"cases": [
{
"description": "empty strand",
"description": "returns zeroes in case of an empty input strand.",
"property": "nucleotideCounts",
"strand": "",
"expected": {
Expand All @@ -17,7 +30,18 @@
}
},
{
"description": "strand with repeated nucleotide",
"description": "can count one nucleotide in single-character input.",
"property": "nucleotideCounts",
"strand": "G",
"expected": {
"A": 0,
"C": 0,
"G": 1,
"T": 0
}
Copy link
Contributor Author

@Vankog Vankog Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

even tough this case might already be included in the next case, this test is for a nice sliming progression. Developers should have quick success by implementing a subset of the spec first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is returning a hardcoded 1 different from returning a hardcoded 7?

Copy link
Contributor Author

@Vankog Vankog Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good point.

I agree that this is not necessary. However, why should anyone hardcode against two or more different test cases? Programming means using your brain to accomplish a task. The TDD cycle is red-green-refactor(?) and sure, you could hardcode the handling of a one-length input and an n-length input to make it green, but then there is still the third phase: To check if there is any need to improve the code. If someone just hardcodes statements to pass several tests, then I dare to say this person might not want to program in the first place.

My intent with the two test cases was, that it is easier to handle a single character input than a sequence. I think the handling of a sequence is an extension of the single char input. Small steps with fast achievements (in form of another green test) seem to be a big motivator to me, especially for inexperienced developers who have to think hard for every step they take.

Copy link
Contributor

@Insti Insti Sep 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I can't see the code context for this comment any more. So I can't comment further on that. (Force-pushing to branches under discussion is a bad idea.)

The TDD cycle is red-green-refactor(?) and sure, you could hardcode the handling of a one-length input and an n-length input to make it green, but then there is still the third phase: To check if there is any need to improve the code. If someone just hardcodes statements to pass several tests, then I dare to say this person might not want to program in the first place.

I disagree with this assessment.

If hardcoded values make the tests pass, then hardcode the values. The refactoring that needs to happen is improving the design of the existing code without altering its behaviour. If after "refactoring" your code can handle more cases than it could previously, you've strayed from the path.

The solution is to add more tests to test the cases that your code does not handle, and then write the code to pass these tests, and then to refactor to improve the design, again without altering its behaviour.

Copy link
Contributor Author

@Vankog Vankog Sep 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@Insti Oh. The context disappeared even after I committed normally. But you can see the context when viewing the first commit directly:
b024822#diff-054c8714b8d69933bb3aed926d01ea6bR32

But it is also in the second commit as well as the overview:
https://github.com/exercism/problem-specifications/pull/895/files#diff-054c8714b8d69933bb3aed926d01ea6bR41

Sorry if I did articulated myself a bit unclear. My paragraph you quoted actually does mean exactly what you argued about: One test at a time, it might be hardcoded at first, but at the refactoring step you should make sure that code smells get cleaned up. That's what I meant with "using the brain".

You argued:

Why is returning a hardcoded 1 different from returning a hardcoded 7?
And here my reply was that: There isn't, but only if you omit the refactoring part, which is an integral part of the cycle.

So my suggestion still stands:
Adding a test for a single-character input
and following up a test for a multi-char input.

reasoning:

  • motivation by fast rewards, in form of making implementation steps between tests smaller.
  • better implementation progression (sliming), because handling a single char is a subset of handling a sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a test for a single-character input
and following up a test for a multi-char input.

This is a good idea.

},
{
"description": "can count one nucleotide in multi-character input.",
"property": "nucleotideCounts",
"strand": "GGGGGGG",
"expected": {
Expand All @@ -28,7 +52,7 @@
}
},
{
"description": "strand with multiple nucleotides",
"description": "can count all nucleotides in a strand containing all nucleotides.",
"property": "nucleotideCounts",
"strand": "AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC",
"expected": {
Expand All @@ -39,12 +63,28 @@
}
},
{
"description": "strand with invalid nucleotides",
"description": "ignores case in a strand of mixed-case nucleotides.",
"property": "nucleotideCounts",
"strand": "AGXXACT",
"strand": "aGcTtTtCaTtCtGaCtGcAaCgGgCaAtAtGtCtCtGtGtGgAtTaAaAaAaGaGtGtCtGaTaGcAgC",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced it's necessary to add the extra functionality (of ignoring case), but I also don't care.

"expected": {
"error": "Invalid nucleotide in strand"
"A": 20,
"C": 12,
"G": 17,
"T": 21
}
},
{
"description": "handles incorrect inputs.",
"cases": [
{
"description": "treats an input with anything else than ACGT-characters as error.",
"property": "nucleotideCounts",
"strand": "AGUU1+ :'A'}CT",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this case is any better or worse than "AGXXACT" at ensuring the description is true. I might even call it worse just for clarity. I would keep AGXXACT. Strength of opinion 2/10.

"expected": {
"error": "Invalid nucleotide in strand"
}
}
]
}
]
}
Expand Down