How to Encrypt Plaintext Properly When it Contains Non-Alphabetical Chars?

Question

My caesar.c doesn't encrypt plaintext properly when there are non-alphabetical chars in my plaintext word. Here's my code for encryption:

int k = atoi(argv[1]);
string plain_text = GetString();

// declare int array of size equal to length of plain text string to use when encrypting with formula
int plain_text_num[strlen(plain_text)];
int n = strlen(plain_text);

for (int i = 0; i < n; i++)
{
    if (isalpha(plain_text[i]) && isupper(plain_text[i]))
    {
        // convert from ASCII to alphabetical index for cap letters
        plain_text_num[i] = plain_text[i] - 65;
    }

    else if (isalpha(plain_text[i]) && islower(plain_text[i]))
    {
        // convert from ASCII to alphabetical index for lower case
        plain_text_num[i] = plain_text[i] - 97;
    }    
    else
    {
        break;
    }
}

for (int j = 0; j < n; j++)
{
    plain_text_num[j] = (plain_text_num[j] + k) % 26;
}

for (int x = 0; x < n; x++)
{
    if (isalpha(plain_text[x]) && isupper(plain_text[x]))
    {
        // convert from alphabetical index to ASCII for cap letters
        plain_text_num[x] = plain_text_num[x] + 65;
    }

    else if (isalpha(plain_text[x]) && islower(plain_text[x]))
    {
        // convert from alphabetical index to ASCII for lower case
        plain_text_num[x] = plain_text_num[x] + 97;
    }    
    else
    {
        // store non-alphabetical chars in int array
        plain_text_num[x] = plain_text[x];
    }
}

for (int y = 0; y < n; y++)
{
    printf("%c", plain_text_num[y]);
}

Here's my program's output when I try to cipher the plaintext string "world, say hello!" with 12 as a key:

The letters 'a' and 'e' get encrypted differently every time I run caesar. "say hello" itself is also encrypted wrongly. But when I run caesar with "worldsayhello!", I get what check50 expects.

Answer 1

As others have said, you can reduce your loops by combining what they do into one loop. You don't need to store the user input, so you can modify it without worry.

Other than that, I would just add that your condition checking could also be simplified. You do not have to check if a char is an alpha before you check if it is an upper case or lower case letter. You need minimum 2 checks for 3 branches:

1. If it is an upper case letter, convert it using the upper case formula.
2. If it is a lower case letter, convert it using the lower case formula.
3. If it is neither, do nothing. There is no need to check if it is alpha because it was neither an upper case or lower case letter, which includes the set of all alphas.

You can use that to structure your algorithm to do it for every char in the array.

Answer 2

I think you are also counting the space character " ".

Try to use the function isalpha() in loop to check whether the current positioned character is an Uppercase or Lowercase letter.

For detail isalpha(), write in terminal:

man isalpha

Answer 3

First this first, you are not wrong with the logic. After going through your code I thing there are few things you need to take care of.

To start with,

if (argc != 2)
{
    printf("Usage: ./caesar <key>\n");
    return 1;
}

// checks that key > 0
else if (atoi(argv[1]) < 0)
{
    printf("Key must be a non-negative integer.\n");
    return 2;
}

Say user enters ./caesar -2 you program will check for 2 command line argument and won't check if its a positive int or not. Since its a else if condition.

You can fix it using && operator or two separate if condition.

Next, String is itself a character array with NULL char ('\0') at the end. So you really don't need to store that one in a separate int array.

But yes, you will need its length; and you did that one rightly.

Next, You ran three different loop. One to change the ASCII code to alphabet number. And second one is to encrypt and then back to ASCII code.

Try to merge them all. Because your way will try to encrypt white spaces as well as numbers in the plain text (keep in mind the spec of PSET).

Some pseudo-code,

Make changes if plain_text[i] isalpha and isupper
{
    plain_text[i]=((plain_text[i] + k - 65) % 26) + 65
}

And

Make changes if plain_text[i] isalpha and islower
{
    plain_text[i]=((plain_text[i] + k - 97) % 26) + 97
}

You can even make use of nested if. This way compiler will enter into the condition only if its a alphabet and then check if its a lower case or upper case as follows.

if(isalpha(plain_text[i])
{
    if(isupper(plain_text[i])
    {
        //code
    }
    else if(islower(plain_text[i])
    {
        //code
    }
}

This way if plain_text[i] is a white space or some number, code will have no effect.

Answer 4

First of all, in these two statements

int plain_text_num[strlen(plain_text)];
int n = strlen(plain_text);

Here are two things,

1. you're calling strlen() twice passing in plaintext, although the result is the same (since the length of plaintext doesn't change) and that's not the best thing to do.

2. although it's not a big deal and this is correct too, but we don't really need to store the ciphered characters since we're not gonna use them afterwards. We only need to print them out and we can do this directly.

Next, you're walking through your plaintext character by character, checking whether the current character is an alphabetical AND uppercase or lowercase. Instead of repeating isalpha(), you could have something like

// if the character is an alphabetical
if (isalpha(plaintext[i]))
{
    // if the character is an uppercase
    if (isupper(plaintext[i]))
    {
        // do something
    }
    // if the character is a lowercase
    else
    {
        // do something else
    }
}

You're then subtracting 65 and 97 respectively (would be more readable if you subtracted 'A' and 'a'). However, if none of the previous conditions is met, you're breaking the loop even of the word still has characters left!

The thing is that you should totally ignore the other case (i.e., the case in which the character is not an alphabetical character) since we're not encrypting them. You may either directly store them in their correct indexes in plain_text_number or you may print them directly if you're not storing the ciphered chars, but not to break the loop if you met one.

Then you're looping through your plain_text_num, adding the key and taking the reminder of dividing by 26. And you're, again, looping through plain_text_num one more time, repeating similar code as above and adding '65' and '97' in case of uppercase/lowercase characters respectively. Again, no issues with that, but this could have been done within the first loop. For example, you could have written something like

plain_text_num[i] = (plain_text[i] - 'A' + k) % 26 + 'A'; // in case of uppercase letters
plain_text_num[i] = (plain_text[i] - 'A' + k) % 26; + 'a' // in case of uppercase letters

And you're having a last loop that prints the ciphered characters out on the screen. This too could have been done in the first loop. So you could have saved 3 loops using only a single loop to cipher your plaintext! :)

Summary:

• The main error is that you're breaking the first loop in case you met a non-alphabetical character, leaving behind the left chars (if any).
• All the other things are just a matter of making your program more readable, understandable and efficient.