Skip to content
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

Fix ROT13 false behavior #1818

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix ROT13 false behavior #1818

wants to merge 2 commits into from

Conversation

ItisCaleb
Copy link

ROT13 will produce incorrect result when rotating numbers with negative amount.

Example

At current version, rotating A0123 with rotate amount -2 will produce the result Y4567

But the correct result is Y8901

ItisCaleb and others added 2 commits May 29, 2024 18:43
ROT13 will produce false result when rotating numbers with negative amount.
@a3957273
Copy link
Member

Nice catch, guess we never thought about putting negative numbers into the ROT13 converter for numbers! Would you be willing to add a new test for this operation to test negative numbers?

@@ -75,7 +77,7 @@ class ROT13 extends Operation {
chr = (chr - 97 + amount) % 26;
output[i] = chr + 97;
} else if (rotNumbers && chr >= 48 && chr <= 57) { // Numbers
chr = (chr - 48 + amount) % 10;
chr = (chr - 48 + numAmount) % 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given numAmount is only used in this case, it might be easier to define it locally in this section, e.g:

                } else if (rotNumbers && chr >= 48 && chr <= 57) { // Numbers
                    const numAmount = 10 - (Math.abs(numAmount) % 10);
                    chr = (chr - 48 + numAmount) % 10;
                    output[i] = chr + 48;
                }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants