-
Notifications
You must be signed in to change notification settings - Fork 18.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: redesign ColumnarTranspositionCipher #5223
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static String decrypt(String message, String key) { | |
public static String decrypt(final String message, final String key) { |
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
encrypt
test (in its current form), - add test cases with empty
word
and/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