[#1475] Update Cipher Tests#1639
Conversation
# Conflicts: # exercises/binary-search/build.gradle # exercises/binary-search/src/test/java/BinarySearchTest.java
|
@FridaTveit how to detect the checkstyle errors? |
|
See our contributing guide. You can run the plugin locally by running |
|
I seem to be getting a |
|
@FridaTveit gentle reminder. 🙂 |
|
@RonakLakhotia We are all maintaining this as volunteers in our spare time and therefore it will sometimes take a while for us to get back to you. Please be patient 🙂 |
My apologies. |
|
rectified the checkstyle errors. 🙂 |
FridaTveit
left a comment
There was a problem hiding this comment.
Looks great, thanks @RonakLakhotia! 🙂
I've left a few comments, let me know if you have any questions or disagree with any of it 🙂
| for (int i = 0; i < Math.min(copy.length(), key.length()); i++) { | ||
| ciphertext.append(encodeCharacter(copy, i)); | ||
| } | ||
| copy = plainText.substring(ciphertext.length(), plainText.length()); |
There was a problem hiding this comment.
Why use this instead of the for loop?
There was a problem hiding this comment.
You mean for loop instead of substring?
| } | ||
| copy = ""; | ||
| for (int i = ciphertext.length(); i < plainText.length(); i++) { | ||
| copy = copy + plainText.charAt(i); |
There was a problem hiding this comment.
Like this you mean?
There was a problem hiding this comment.
No, sorry if I wasn't clear. I meant why did this have to change at all? Why not keep the for loop as it used to be? Does that not work with the changes to the tests? 🙂
There was a problem hiding this comment.
With the original code, the cipherMessageLongerThanKey test case seems to be failing in SimpleCipherStepTwoSubsitutionTest
There was a problem hiding this comment.
Oh I see 🙂 Well, I think adding the extra while loop makes it a bit confusing? Maybe it would be easier to add a small helper method which will pad or cut the key so that it's the same length as the plaintext? That is something like:
String sameLengthKey = getKeySameLengthAsPlaintext(plaintext, key);
for (int index = 0; index < plaintext.length(); index++) {
ciphertext.append(encodeCharacter(plaintext, index, key));
}
What do you think? 🙂 It's not very important though, so feel free to keep it as it is if you like. But if so please change it back to use substring 🙂
There was a problem hiding this comment.
@FridaTveit made the relevant changes as you suggested. 👍
FridaTveit
left a comment
There was a problem hiding this comment.
This looks great! Thanks for making those changes @RonakLakhotia 🙂 I've left a few comments but they're all very minor things 🙂
FridaTveit
left a comment
There was a problem hiding this comment.
Looks great, thanks @RonakLakhotia 🙂
Thanks for the guidance @FridaTveit 👍 |
This test file was removed years ago in exercism#1639, but the instructions still mention it (which caused me, at least, quite a bit of confusion since I thought I had deleted something erroneously). This fixes the documentation to remove the reference to Step Three. If the intent was to never have deleted SimpleCipherStepThreeTest.java in the first place, let me know and I'll amend the PR to that.
This test file was removed years ago in exercism#1639, but the instructions still mention it (which caused me, at least, quite a bit of confusion since I thought I had deleted something erroneously). This fixes the documentation to remove the reference to Step Three. If the intent was to never have deleted SimpleCipherStepThreeTest.java in the first place, let me know and I'll amend the PR to that.
…2523) * [simple-cipher] Fix outdated reference to SimpleCipherStepThreeTest This test file was removed years ago in #1639, but the instructions still mention it (which caused me, at least, quite a bit of confusion since I thought I had deleted something erroneously). This fixes the documentation to remove the reference to Step Three. If the intent was to never have deleted SimpleCipherStepThreeTest.java in the first place, let me know and I'll amend the PR to that. * [simple-cipher] Fix incorrect reference to test file in instructions. This revealed that actually there's a typo in the class name itself. Determining next whether to fix that or leave it be. * [simple-cipher] Combine the two test classes into a single suite * [simple-cipher] Fix checkstyle failures * [simple-cipher] Remove outdated test files from .meta/config.json * [simple-cipher] Use Enclosed JUnit runner for inner test classes
Fixes #1475