Repository navigation
Conversation
There was a problem hiding this comment.
This looks really good @anu0012 :) I've suggested a few more topics but feel free to disagree with me :)
A couple of suggestions for the future. Please only tackle one issue per PR. Doing several issues per PR makes it harder to review and it means additions that are good to go can be held up by changes needed elsewhere. For example, if you had made these separate PRs I would have been able to merge the topics for bracket-push and atbash-cipher straight away.
Also, it's a good idea to do your PR changes on a separate branch on your fork, not on your master branch. Any commit you now push on your master branch will appear in this PR which means it's easy to accidentally push unrelated things to your PR, and it's harder for you to keep doing other work while your PR is being reviewed.
Those are just minor things to bear in mind for the future. Thanks for taking the time to contribute :)
| "strings", | ||
| "cryptography", | ||
| "security", | ||
| "mathematics" |
There was a problem hiding this comment.
I would add randomness and exception_handling as well.
There was a problem hiding this comment.
Yes, exception_handling can be used also but at the same time, we can check the input manually without worrying about the exception. Actually, I didn't think in a language-specific way.
There was a problem hiding this comment.
Again the tests expect an exception to be thrown so you have to use exceptions. These are meant to be language specific topics for doing the exercises on the java track :)
| "text_formatting", | ||
| "mathematics", | ||
| "cryptography", | ||
| "security" |
There was a problem hiding this comment.
I would add lists as well
There was a problem hiding this comment.
I think it can be done without using lists also. Strings can be separated using Split() function also.
There was a problem hiding this comment.
No, the getPlaintextSegments() method needs to return a List in the tests so you have to use lists :)
There was a problem hiding this comment.
Alright. I'll add it. Thanks for the correction :)
There was a problem hiding this comment.
No problem :) Thanks for explaining your reasoning :)
|
@FridaTveit Thanks a lot for your valuable suggestions. I'll definitely take care of those in future. Just one more thing should I make another PR for each issue separately? |
|
If you make any more PRs to fix other issues then yes :) But for this one you can keep it as it is since I've already reviewed it and I can imagine it would be a bit of a pain for you to try and split it into four PRs now :) You could do if you wanted to though, I don't particularly mind :) |
|
@FridaTveit If possible you can merge this, I'll be more careful in my future PRs :) |
|
Sure. But could you add the topics I suggested first? Or explain why you don't think they don't should be added :) |
|
This has been fixed in PR #910. |
2 participants
…cipher
Fixes #899 #893 #892 #877
Reviewer Resources:
Track Policies