Improve readability of ciphers/mixed_keyword_cypher.py by yanvoi · Pull Request #8626 · TheAlgorithms/Python

yanvoi · 2023-04-08T14:23:59Z

Describe your change:

I've tried my best with refactoring the code - it still might be too little but that's how much I've managed to do with the time I had.

Add an algorithm?
Fix a bug or typo in an existing algorithm?
Documentation change?

Checklist:

cclauss · 2023-04-09T06:38:38Z

ciphers/mixed_keyword_cypher.py

+    unique_chars = []
+    for char in keyword:
+        if char not in unique_chars:
+            unique_chars.append(char)


Please make this a set.

https://docs.python.org/3/library/stdtypes.html#set

I believe it cannot be done with sets because the order of these characters in the keyword matters - it determines how we will map plaintext characters to the cyphertext.

I believe it cannot be done with sets because the order of these characters in the keyword matters - it determines how we will map plaintext characters to the cyphertext.

Then I think it would help if this was mentioned in the comments, so that future maintainers know that the char order matters

cclauss · 2023-04-09T06:40:44Z

ciphers/mixed_keyword_cypher.py

-        s = []
-        for _ in range(len_temp):
-            s.append(temp[k])
+    modified_alphabet = []


Please do all this with set

As this is a basic substitution cypher, it is bounded by the number of different letters in the alphabet we're using. Using sets in this case wouldn't change the performance almost at all.

cclauss · 2023-04-09T06:45:56Z

On your local machine, please run python3 -m doctest -v ciphers/mixed_keyword_cypher.py
because pytest is failing on:

=================================== FAILURES ===================================
_____________ [doctest] ciphers.mixed_keyword_cypher.mixed_keyword _____________
008     H E L O
009     A B C D
010     F G I J
011     K M N P
012     Q R S T
013     U V W X
014     Y Z
015     and map vertically
016 
017     >>> mixed_keyword("college", "UNIVERSITY")  # doctest: +NORMALIZE_WHITESPACE
Expected:
    {'A': 'C', 'B': 'A', 'C': 'I', 'D': 'P', 'E': 'U', 'F': 'Z', 'G': 'O', 'H': 'B',
     'I': 'J', 'J': 'Q', 'K': 'V', 'L': 'L', 'M': 'D', 'N': 'K', 'O': 'R', 'P': 'W',
     'Q': 'E', 'R': 'F', 'S': 'M', 'T': 'S', 'U': 'X', 'V': 'G', 'W': 'H', 'X': 'N',
     'Y': 'T', 'Z': 'Y'}
    'XKJGUFMJST'
Got:
    'XKJGUFMJST'

yanvoi · 2023-04-10T07:15:37Z

It turned out that whoever has written this algorithm hard-coded the number of rows into it.

…in the alphabet

tianyizheng02 · 2023-05-09T07:44:03Z

ciphers/mixed_keyword_cypher.py

+    alphabet = list(ascii_uppercase)
+    alphabet_set = set(alphabet)


Suggested change

alphabet = list(ascii_uppercase)

alphabet_set = set(alphabet)

alphabet_set = set(ascii_uppercase)

... plus other changes to remove references to alphabet

I think converting ascii_uppercase to a list is unnecessary. Converting to a set, iterating, and indexing should all still work if you use the imported string directly.

I can of course change that but I wanted to make it easier for people who would like to use other alphabets (e.g. cyrillic) for their ciphers. So it is your call - should I change it or not?

If people want to use other alphabets such as Cyrillic, then they would have to go into the source code and change ascii_uppercase to a different variable. That doesn't really address the issue that ascii_uppercase (or whatever the user replaces it with) still doesn't need to be converted to a list.

If you want it to be easier to switch to a different alphabet, then perhaps you could have the function take in the alphabet string (ascii_uppercase, etc) as an input parameter

Fair point - I've done both - Now there's no conversion to the list and a different alphabet can be passed to the function. Thank you for the review!

for more information, see https://pre-commit.ci

yanvoi · 2023-05-12T09:58:07Z

The tests seem to be failing due to code outside of the file I've been editing.

tianyizheng02 · 2023-05-12T10:18:14Z

The tests seem to be failing due to code outside of the file I've been editing.

Other contributors have been experiencing the same issue with ruff... try running ruff check --fix . to see if ruff can automatically fix any of the linting issues

yanvoi · 2023-05-12T12:55:08Z

Running ruff resulted in conflicts with another branch - I've resolved them and then ran ruff again and nothing changed - ruff finds 4 errors but doesn't fix them automatically.

tianyizheng02 · 2023-05-16T10:44:52Z

@yanvoi I believe the ruff errors were fixed in #8732, so the ruff check should pass after you update your branch

* Update rsa_cipher.py by replacing %s with {} * Update rsa_cipher.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update linear_discriminant_analysis.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update linear_discriminant_analysis.py * Update linear_discriminant_analysis.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update linear_discriminant_analysis.py * Update linear_discriminant_analysis.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update linear_discriminant_analysis.py * Update machine_learning/linear_discriminant_analysis.py Co-authored-by: Christian Clauss <cclauss@me.com> * Update linear_discriminant_analysis.py * updated --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Christian Clauss <cclauss@me.com>

tianyizheng02 · 2023-05-17T04:47:30Z

@cclauss How is it that @rohan472000's changes in their own branch are making their way into this PR?

for more information, see https://pre-commit.ci

tianyizheng02 · 2023-05-17T04:54:29Z

Oh my goddddd I knew this was gonna happen...

@yanvoi The ruff error is once again in another file that you didn't edit, but I already fixed it in #8743. Once that PR gets merged, updating your branch on GitHub should finally fix the issue

yanvoi · 2023-05-17T10:19:44Z

@tianyizheng02 No worries, I'll update when #8743 gets merged

tianyizheng02 · 2023-05-22T05:21:26Z

@yanvoi The ruff error was fixed in #8746 instead, so you should be good to update your branch

yanvoi · 2023-05-22T17:19:28Z

@tianyizheng02 All done, thanks again!

tianyizheng02 · 2023-06-08T00:17:53Z

ciphers/mixed_keyword_cypher.py

+            mapping[alphabet[letter_index]] = row[column]
+            letter_index += 1
+
+    print(mapping)


Ideally the algorithm shouldn't have side effects such as printing, but I understand that printing the mapping is helpful for debugging purposes. Perhaps it'd be better to have a "verbose" function parameter that allows users to enable/disable printing the mapping

Done - I've also added a test case that includes setting the verbose parameter to False

cclauss · 2023-06-08T07:02:16Z

@tianyizheng02 Please approve this PR if you believe that it is ready to merge.

ciphers/mixed_keyword_cypher.py

Co-authored-by: Tianyi Zheng <tianyizheng02@gmail.com>

Starmania · 2023-06-09T09:27:50Z

Thanks you for the edit yanvoi !

refactored the code

518f4e5

algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files tests are failing Do not merge until tests pass labels Apr 8, 2023

cclauss reviewed Apr 9, 2023

View reviewed changes

cclauss changed the title ~~fixes: #8622~~ Improve readability of ciphers/mixed_keyword_cypher.py Apr 9, 2023

the code will now pass the test

16c9268

algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Apr 9, 2023

yanvoi requested a review from cclauss April 9, 2023 10:07

looked more into it and fixed the logic

a6d64ff

yanvoi added 2 commits April 10, 2023 10:35

made the code easier to read, added comments and fixed the logic

642ccc3

got rid of redundant code + plaintext can contain chars that are not …

3520c16

…in the alphabet

tianyizheng02 requested changes May 10, 2023

View reviewed changes

yanvoi requested a review from tianyizheng02 May 10, 2023 13:00

tianyizheng02 mentioned this pull request May 11, 2023

Documentation #8646

Closed

14 tasks

yanvoi and others added 2 commits May 12, 2023 09:04

fixed the reduntant conversion of ascii_uppercase to a list

3a099f5

[pre-commit.ci] auto fixes from pre-commit.com hooks

738f193

for more information, see https://pre-commit.ci

algorithms-keeper bot added the tests are failing Do not merge until tests pass label May 12, 2023

keyword and plaintext won't have default values

b284ab1

tianyizheng02 approved these changes May 12, 2023

View reviewed changes

ran the ruff command

ad436ce

Merge branch 'master' into issue8622fix

dbbd778

yanvoi requested a review from tianyizheng02 May 12, 2023 13:09

yanvoi and others added 2 commits May 17, 2023 06:49

fixed some difficulties

33b34c4

[pre-commit.ci] auto fixes from pre-commit.com hooks

66e28ee

for more information, see https://pre-commit.ci

Merge branch 'master' into issue8622fix

35d3de2

algorithms-keeper bot removed the tests are failing Do not merge until tests pass label May 22, 2023

tianyizheng02 requested changes Jun 8, 2023

View reviewed changes

yanvoi added 2 commits June 8, 2023 08:13

added comments, made printing mapping optional, added 1 test

0bf2ed2

shortened the line that was too long

05d0283

algorithms-keeper bot added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels Jun 8, 2023

yanvoi requested a review from tianyizheng02 June 8, 2023 06:27

tianyizheng02 requested changes Jun 9, 2023

View reviewed changes

ciphers/mixed_keyword_cypher.py Outdated Show resolved Hide resolved

Update ciphers/mixed_keyword_cypher.py

ad8c99f

Co-authored-by: Tianyi Zheng <tianyizheng02@gmail.com>

yanvoi requested a review from tianyizheng02 June 9, 2023 07:32

tianyizheng02 approved these changes Jun 9, 2023

View reviewed changes

cclauss enabled auto-merge (squash) June 9, 2023 09:06

cclauss approved these changes Jun 9, 2023

View reviewed changes

cclauss merged commit 9c9da8e into TheAlgorithms:master Jun 9, 2023

tianyizheng02 mentioned this pull request Jul 31, 2023

Refactor mixed_keyword_cypher.py file and added comments in code #8633

Closed

14 tasks

isidroas mentioned this pull request Jan 25, 2025

dh isidroas/Python#1

Closed

14 tasks

		alphabet = list(ascii_uppercase)
		alphabet_set = set(alphabet)

	alphabet = list(ascii_uppercase)
	alphabet_set = set(alphabet)
	alphabet_set = set(ascii_uppercase)

Uh oh!

Conversation

yanvoi commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your change:

Checklist:

Uh oh!

cclauss Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianyizheng02 Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss commented Apr 9, 2023

Uh oh!

yanvoi commented Apr 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanvoi commented May 12, 2023

Uh oh!

tianyizheng02 commented May 12, 2023

Uh oh!

yanvoi commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianyizheng02 commented May 16, 2023

Uh oh!

tianyizheng02 commented May 17, 2023

Uh oh!

tianyizheng02 commented May 17, 2023

Uh oh!

yanvoi commented May 17, 2023

Uh oh!

tianyizheng02 commented May 22, 2023

Uh oh!

yanvoi commented May 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss commented Jun 8, 2023

Uh oh!

Uh oh!

Starmania commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yanvoi commented Apr 8, 2023 •

edited

Loading

cclauss Apr 9, 2023 •

edited

Loading

tianyizheng02 Jun 8, 2023 •

edited

Loading

yanvoi commented May 12, 2023 •

edited

Loading