Protein translation#365
Conversation
matthewmorgan
left a comment
matthewmorgan
left a comment
There was a problem hiding this comment.
@RobinCsl looks great! I had a couple of style suggestions, but nothing critical. Change if you like, if not, I'm OK with things as they are.
| }); | ||
|
|
||
| xtest('Invalid codon throws error', () => { | ||
| expect(() => { translate('LOL'); }).toThrow(new Error('Invalid codon')); |
There was a problem hiding this comment.
We could clean this up a little like so:
expect(() => translate('LOL')).toThrow(new Error('Invalid codon'));| default: | ||
| return 'INVALID'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nothing at all wrong with the massive switch statement, but I sometimes like an alternative approach to these sorts of things, using an object. EG:
const ACID_PROTEIN_MAP = {
'AUG': 'Methionine',
'UUU': 'Phenylalanine',
'UUC': 'Phenylalanine',
// and so on...
};
const getProtein = codon => ACID_PROTEIN_MAP[codon] || 'INVALID';It remains very readable and gets what's essentially configuration out of your logic. In production, you might pull the config out even further: this makes it easy to adjust the mapping without having to redeploy code.
|
Implemented the changes proposed. I tried to extract the config in an external json file, but it seemed to cause more complications, so I finally opted for the easier solution of setting this variable in the same file. |
| "UAA": "STOP", | ||
| "UAG": "STOP", | ||
| "UGA": "STOP" | ||
| } |
There was a problem hiding this comment.
Gorgeous! Awesome indeed.
|
@RobinCsl I see why you're having a problem-- I think the makefile that runs the test suite does not copy your config file over...not a problem to include it in the example file. It's nice, though, right? The config is ~ 1/2 the size of the This PR looks good to me, if you could please squash the commits we'll merge it. |
|
Extract configuration to external files is usually a good idea in the real world. In exercism we try to get just three files: readme, tests and production/solution. Keeping all tests and all solution code in one file each make solution management much easy. I think that could be the reason why the Sorry @RobinCsl for causing you some struggles with that. And thanks for implementing this exercise for the track. |
RobinCsl
force-pushed
the
protein-translation
branch
from
0865614 to
1a29710
Compare
RobinCsl
force-pushed
the
protein-translation
branch
from
559b7d9 to
2931760
Compare
|
I tried to squash all the commits together, but every time I push there would still be a conflict with master, even though I rebased off upstream/master. So I believe I will need assistance with squashing my commits. Sorry! |
RobinCsl
force-pushed
the
protein-translation
branch
from
90c87f7 to
980058d
Compare
RobinCsl
force-pushed
the
protein-translation
branch
from
980058d to
4e42bdd
Compare
|
Looks like I managed now, I guess I had not rebased in the right way. |
RobinCsl
force-pushed
the
protein-translation
branch
from
4e42bdd to
816f3a7
Compare
|
No problem, should be fine now :) Thank you! |
|
@RobinCsl glad to hear you got it sorted. I did not see you were having trouble, hope it wasn't too frustrating. Sometimes it can be a bit tricky to resolve the conflicts. Thanks for the contribution! |
Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (exercism#366) [Housekeeping] Upgrade jest (exercism#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (exercism#362) Transpose Exercise is ECMAScript ready (exercism#364) Upgrade jest Protein Translation is ECMAScript ready (exercism#365) (exercism#365) Update setup links in all READMEs (exercism#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes exercism#370 Updated setup link
Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (exercism#366) [Housekeeping] Upgrade jest (exercism#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (exercism#362) Transpose Exercise is ECMAScript ready (exercism#364) Upgrade jest Protein Translation is ECMAScript ready (exercism#365) (exercism#365) Update setup links in all READMEs (exercism#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes exercism#370 Updated setup link
Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (#366) [Housekeeping] Upgrade jest (#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (#362) Transpose Exercise is ECMAScript ready (#364) Upgrade jest Protein Translation is ECMAScript ready (#365) (#365) Update setup links in all READMEs (#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes #370 Updated setup link
Grouped all tests under a single describe Ordered the tests in a more logical way. All tests except the first one are skipped. Removed mention of enabling tests by changing xdescribe to describe. Implement exercise isbn-verifier (exercism#366) [Housekeeping] Upgrade jest (exercism#368) * Use `package-lock.json` instead of `yarn.lock` * Upgrade jest and babel-jest to latest versions Jest to 21.2.1 babel-jest to 21.2.0 Change Exercise is now ECMAScript ready (exercism#362) Transpose Exercise is ECMAScript ready (exercism#364) Upgrade jest Protein Translation is ECMAScript ready (exercism#365) (exercism#365) Update setup links in all READMEs (exercism#371) Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation Closes exercism#370 Updated setup link
3 participants
Same comment as for PR #364, I did some kind of manual rebase so that there shouldn't be any conflicts. Looking forward to having your comments.