Refactor: redesign ColumnarTranspositionCipher #5223
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5223 +/- ##
============================================
+ Coverage 40.35% 40.68% +0.32%
- Complexity 2486 2501 +15
============================================
Files 519 519
Lines 15479 15440 -39
Branches 2950 2943 -7
============================================
+ Hits 6247 6282 +35
+ Misses 8943 8867 -76
- Partials 289 291 +2 ☔ View full report in Codecov by Sentry. |
|
Please focus on #5221 first. |
samuelfac
requested review from
yanglbme,
vil02 and
BamaCharanChhandogi
as code owners
| for (int i = 0; i < sortedTable[0].length; i++) { | ||
| for (int j = 1; j < sortedTable.length; j++) { | ||
| wordEncrypted.append(sortedTable[j][i]); | ||
| public static String encrypt(String message, String key) { |
There was a problem hiding this comment.
| public static String encrypt(String message, String key) { | |
| public static String encrypt(final String message, final String key) { |
| wordEncrypted.append(sortedTable[j][i]); | ||
| public static String encrypt(String message, String key) { | ||
| int numColumns = key.length(); | ||
| int numRows = (int) Math.ceil((double) message.length() / numColumns); |
There was a problem hiding this comment.
This deserves a separate method, especially because it is used in decrypt.
| for (int i = 1; i < table.length; i++) { | ||
| for (Object item : table[i]) { | ||
| wordDecrypted.append(item); | ||
| public static String decrypt(String message, String key) { |
There was a problem hiding this comment.
| public static String decrypt(String message, String key) { | |
| public static String decrypt(final String message, final String key) { |
There was a problem hiding this comment.
hey, I am currently working on #5223, I think the decrypt method does not need any parameters, because we have included global variables to store the respective..
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| void encrypt(String key, String word) { | ||
| assertNotEquals(word, ColumnarTranspositionCipher.encrypt(word, key)); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| void decrypt(String key, String word) { | ||
| String encrypted = ColumnarTranspositionCipher.encrypt(word, key); | ||
| assertEquals(word, ColumnarTranspositionCipher.decrypt(encrypted, key)); | ||
| } | ||
|
|
||
| private static Stream<Arguments> provideTestCases() { | ||
| return Stream.of(Arguments.of("asd215", "This is a test of the Columnar Transposition Cipher"), // | ||
| Arguments.of("test123456", "test"), // | ||
| Arguments.of("my long secret pass 1234567890", "short message"), // | ||
| Arguments.of("secret", "My secret message") // | ||
| ); | ||
| } |
There was a problem hiding this comment.
These tests check only that ColumnarTranspositionCipher.encrypt is not an identity and that encrypt is an inverse of decrypt. I would highly suggest to:
- add the expected into the test cases, i.e. each test case should consists of
message,key,encrypted. - remove the
encrypttest (in its current form), - add test cases with empty
wordand/orkey, - (as discussed earlier) remove the
//
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution! |
|
Please reopen this pull request once you have made the required changes. If you need help, feel free to ask in our Discord server or ping one of the maintainers here. Thank you for your contribution! |
Related to #5164
clang-format -i --style=file path/to/your/file.java