Protein Translation: Implement problem in Java language stream. #985
Conversation
There was a problem hiding this comment.
This is looking really good :)
There are a couple of important things you need to do:
-
Add an entry for this exercise to settings.gradle. See the contributing guide for a list of things that need doing when adding a new exercise, which includes adding an entry to settings.gradle :)
-
Add starter implementation instead of a .keep file. The current policy for adding starter implementation is that any of the first 20 exercises on the track should have it. See our POLICIES doc for a description of this policy. We're about to change this so that any exercise with a difficulty of 4 or lower should have starter implementation. But either way this exercise should have starter implementation :)
In general it's a good idea to consult those two documents before implementing an exercise :)
Also, it would be great if you could suggest a core exercise which could unlock this exercise. Generally this should be any core exercise that uses topics similar to the ones used in this exercise. You don't have to do that, but it would be useful if you fancied it :)
And just for next time, it might be useful to wait until the canonical data PR has been merged. That way we could add a version file and make sure the tests stayed up to date with the canonical data. But like you said, it's not essential :)
Overall this looks really good, thanks for taking the time to do it! :)
| @Test | ||
| public void testMethionineRnaSequence() { | ||
| List<String> expected = Arrays.asList("Methionine"); | ||
| assertEquals( expected , proteinTranslator.translate("AUG")); |
There was a problem hiding this comment.
There shouldn't be any whitespace before and after expected :)
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class ProteinTranslator { |
There was a problem hiding this comment.
This could be package private :)
|
That's great! I'm not sure why I blanked on the |
There was a problem hiding this comment.
Thanks for making those changes so quickly @sjwarner-bp! :) I've left a couple of comments about some minor issues but apart from that it looks ready to merge! :D
|
|
||
| class ProteinTranslator { | ||
|
|
||
| List<String> translate (String rnaSequence) { |
There was a problem hiding this comment.
There shouldn't be a space after translate :)
| private static final String[] TRYPTOPHAN = {"UGG"}; | ||
| private static final String[] STOP = {"UAA", "UAG", "UGA"}; | ||
|
|
||
| List<String> translate (String rnaSequence) { |
There was a problem hiding this comment.
There shouldn't be a space after translate :)
|
|
||
| List<String> translate (String rnaSequence) { | ||
|
|
||
| List proteinList = new ArrayList(); |
There was a problem hiding this comment.
This should be List<String> proteinList = new ArrayList<String>(). Otherwise you're using a list of generic Objects which isn't typesafe. Does that make sense? :)
|
Oops! Spaces are always a silly thing for me, especially without using linters! The typesafety thing makes sense too - I was reading about it not too long ago 🙂 |
There was a problem hiding this comment.
This looks great @sjwarner-bp! Thanks for implementing this! :)
And especially thanks for responding to comments so promtly and being so willing to take on board changes. That makes it a pleasure to review your code! :)
|
Not sure why the build failed. I've restarted it. Sometimes it just fails for no reason and then runs fine when we restart it so we'll see :P |
|
The build is fine now so it doesn't look like it was because of anything you did :) |
|
Perfect, thank you 😄 |
After adding the
canonical-data.jsonto problem-specifications here, I wanted to implement the problem in Java. I assume that the fact there was no canonical data was the only reason this problem is yet to be implemented.Please bear in mind that the
canonical-data.jsonhas not yet been merged, though it is approved. Hopefully this should not impact this PR.Reviewer Resources:
Track Policies