nucleotide-count: add generator and update example #1014
Conversation
There was a problem hiding this comment.
Looking good overall.
Consider using map[byte]int as the .expected type. Doing so would also allow retaining the stub file nucleotide_count.go as it is; then I think adding a .meta/hints.md file would be helpful to give instructions about competing the type declarations in the stub file.
@ferhatelmas and @tleen - Want to chime in about this ?
| { | ||
| description: "empty strand", | ||
| strand: "", | ||
| expected: map[string]int{"A": 0, "C": 0, "G": 0, "T": 0}, |
There was a problem hiding this comment.
Would it be easy enough to use this expected type ?
expected map[byte]int
and then generate this as
expected: map[byte]int{'A': 0, 'C': 0, 'G': 0, 'T': 0},
A nucleotide is one single letter after all. Perhaps too nit-picky, but it seems to match the exercise problem better.
What do you think ?
There was a problem hiding this comment.
Seems reasonable to me. Always a fan of more content in hints.md as well.
|
Hi, I've changed the expected type to map[byte]int and added a little hints.md ; the problem in keeping the original nucleotide_count.go is that it suggests to implement a type 'Histogram' ... |
| for i := range validNucleotides { | ||
| nucleotide := validNucleotides[i] | ||
| h[nucleotide], _ = dna.Count(nucleotide) | ||
| total += h[nucleotide] | ||
| result[nucleotide], _ = dna.Count(nucleotide) |
There was a problem hiding this comment.
Perhaps replace
result[nucleotide], _ = dna.Count(nucleotide)
with
if result[nucleotide], e = dna.Count(nucleotide); e != nil {
return nil, e
}
Then var total is unnecessary as well as the later if test using it; it also preserves the specific error from Count.
There was a problem hiding this comment.
That would fail the test with invalid nucleotides, like "AGXXACT" in the test suite... Because the algorithm only iterates on valid ones "ACGT". I'll remove pointless code :)
| @@ -2,7 +2,7 @@ package dna | |||
|
|
|||
| // Histogram is a mapping from nucleotide to its count in given DNA. | |||
| // Choose a suitable data type. | |||
| type Histogram | |||
| type Histogram | |||
There was a problem hiding this comment.
Unnecessary white space change ?
ferhatelmas
changed the title
| strs = append(strs, `'`+k+`': `+strconv.FormatFloat(t, 'f', -1, 64)) | ||
| default: | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah,
default:
That line is unnecessary. Can remove it.
There was a problem hiding this comment.
ok .. I made it a little more useful :)
in the meantime the problem specification json structure has changed, so I updated also the generator
reference issue #605