Search code examples
cstringintegercs50

CS50 Week 2 Caesar Practice

My code seems to be working properly except at the point when it should print the final output. The problem is to input a string and output an encrypted version. The encryption works by adding an int defined as the key and then adding that value to each character of the ascii values of the inputed string. My issue is that when the cypher text is outputted there are only spaces and no letters or even numbers.

#include <cs50.h>
#include <stdio.h>
#include <ctype.h>
#include <math.h>
#include <string.h>
#include <stdlib.h>

int main(int argc, string argv[]) {

    int key = atoi(argv[1]);

    printf("%i\n", key);

    if (argc != 2) {
        printf("Usage: ./ceasar key\n");
    } else {
        string text = get_string("Plaintext: ");

        for (int i = 0, len = strlen(text); i < len; i++) {
            int cipher = text[i];
            int ciphertext = cipher + key;
            int ciphermod = ciphertext % 26;
            printf("%c", ciphermod);
        }
        printf("\n");
    }
}
Solution

  • You've got a few issues going on here. Please make sure to thoroughly read the assignment before turning to others for assistance.

    The assignment requires you to:

    • Only encode alphabetic characters. Look to the function isalpha() for this.
    • Encode both uppercase and lowercase characters accurately. Note that, in ASCII, uppercase letters and lowercase letters are separate entities.
      • Meaning, you must have your code be able to handle both, as they are each handled differently.
      • Perhaps taking some time to sit and take in the ASCII table may be helpful to you, as it will help you understand what is really happening when you add the key.
    • Use the correct formula for encoding letters. The i'th ciphered letter ci corresponding to the i'th plaintext letter pi is defined as ci = (pi + k) % 26.
      • Your code is equivalent to this formula, but it does not account for wrapping, uppercase/lowercase letters, etc. The project specification doesn't just ask you to repeat the formula, it asks you to solve a problem using it. To do so, you must understand it. I explain more, subsequently.

    I recommend:

    • Modifying the text in-place. Currently, you calculate the ciphered text and print it. If you add code for modifying the text where it sits, it'll make ignoring non-alphabetic characters easier.
    • Modify the formula.
      • Where 𝚨 is the ASCII character code for the beginning of either the uppercase or lowercase characters, the formula might shake out as follows:
        • ci = (pi - 𝚨 + k) % 26 + 𝚨
        • What this modified formula does is first take the ASCII code for Pi and turn it into a number that represents which letter in the alphabet it is, ignoring case. Then, you can add the key(shift the cipher). Using % 26 on this result then makes sure that the result is between 1 and 26—always a letter. Finally, we add back 𝚨 so that the character has a case again.

    Here's the modified code with the solution broken down, step by step:

    // ...

    for (int i = 0, n = strlen(text); i < n; i++) {
      if (!isalpha(text[i])) continue;
      if (isupper(text[i])) {
        // the letter's ASCII code on its own.
        int charcode = text[i];

        // the letter's index in the alphabet. A = 0, B = 1, etc. 
        // this is no longer a valid ASCII code.
        int alphabet_index = charcode - 'A';

        // the letter's index in the alphabet, shifted by the key.
        // note, this may shift the letter past the end/beginning of the alphabet.
        int shifted_alphabet_index = alphabet_index + key;

        // the letter's index in the alphabet, shifted by the key, wrapped around.
        // the modulo operator (%) returns the remainder of a division. 
        // in this instance, the result will always be between 0 and 25,
        // meaning it will always be a valid index in the alphabet.
        int shifted_index_within_alphabet = shifted_alphabet_index % 26;

        // this is the final ASCII code of the letter, after it has been shifted.
        // we achieve this by adding back the 'A' offset so that the letter is
        // within the range of the correct case of letters.
        int final_shifted_charcode = shifted_index_within_alphabet + 'A';

        text[i] = final_shifted_charcode;
      }
      else { // islower
        int charcode = text[i];
        int alphabet_index = charcode - 'a';
        int shifted_alphabet_index = alphabet_index + key;
        int shifted_index_within_alphabet = shifted_alphabet_index % 26;
        int final_shifted_charcode = shifted_index_within_alphabet + 'a';
        text[i] = final_shifted_charcode;
      }
    }
    printf("ciphertext: %s\n", text);

// ...

    And here is the solution, simplified down:

    // ...

    for (int i = 0, n = strlen(text); i < n; i++) {
      if (!isalpha(text[i]))                        // if not alphabetic, skip
        continue;                                   //
      if (isupper(text[i]))                         // if uppercase
        text[i] = (text[i] - 'A' + key) % 26 + 'A'; //
      else                                          // if lowercase
        text[i] = (text[i] - 'a' + key) % 26 + 'a'; //
    }
    printf("ciphertext: %s\n", text);


// ...

    Just as a side note, the statement if (!isalpha(text[i])) is acting like something called a guard clause. This is a useful concept to know. Using guard clauses allows you to have simpler, more readable code. Imagine if I had nested all of the code inside the for loop under the if (isalpha(text[i])) condition. It would be harder to read and understand, and difficult to match up the different bracket pairs.

    Edit: I would also echo what chqrlie said. Do not use argv[n] until you have verified that argc >= (n + 1)