*Edited to include the latest changes in the code.

Can someone help me spot the problem with my code? Below are the results from check50 and the code itself. TIA!!

Here is the prior question: PSET 2 Caesar : help debugging caesar : continued #2

enter image description here

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

bool only_digits(string s);
char rotate(char c, int n);

int main(int argc, string argv[])
    // make sure program is run with just one command line argument
    if (argc != 2)
        printf("Usage: ./caesar key\n");
        return 1;

    // make sure every digit in argv[1] is a digit
    if (only_digits(argv[1]) == false)
        printf("Usage: ./caesar key\n");
        return 1;

    // convert argv[1] from a 'string' to an 'int' and store that number in variable "key"
    int key = atoi(argv[1]);

    // prompt user for plaintext
    string plaintext = get_string("plaintext:  ");

    // length of the plaintext
    int text_length = strlen(plaintext);

    // create string for ciphertext with the same length as the plaintext
    char ciphertext[text_length + 1];
    ciphertext[text_length + 1] = 0;

    // for each character in the plaintext
    for (int i = 0; i < text_length; i++)
        // rotate the character only if it's a letter
        if (isalpha(plaintext[i]))
            ciphertext[i] = rotate(plaintext[i], key);
            ciphertext[i] = plaintext[i];

    // print out the ciphertext
    printf("ciphertext: %s\n", ciphertext);
    return 0;


bool only_digits(string s)
    for (int i = 0, j = strlen(s); i < j; i++)
       if (isdigit(s[i]) == false)
            return false;
    return true;

char rotate(char c, int n)
    char rotated_char;
    if (isupper(c))
        // rotate n number of places forward in the alphabet, uppercase, A = 65, lowercase, a = 97
        rotated_char = ((c - 'A') + n) % 26;
        // return the result
        c = rotated_char + 'A';
    else if (islower(c))
        rotated_char = ((c - 'a') + n) % 26;
        c = rotated_char + 'a';
        rotated_char = c;
    return rotated_char;

1 Answer 1


No once again you did not follow instructions and you broke your code and repeated the same series of problems I just described to you. Before I say anything else, if you do not start following instructions and making just the changes I ask I am done helping you. It is a waste of time, I can't debug a moving target and it has become evident you have no idea what you are doing, and insist on doing your own thing at every turn.

You corrected the size of the string ciphertext; but you changed the null terminator assignment and broke it in the process. You can't write to that address because it is outside the bounds of your array. Please go back and watch the video lesson again. You either haven't watched it, or failed to grasp the concepts around strings while watching it.

Here's a demonstration of why what you did is wrong since you clearly don't understand how char arrays or strings work.

Let's pretend plaintext was entered as foo. strlen(plaintext) will equal 3, but in memory its size is 4. string length != memory size of the string. Let's look at the memory layout of how plaintext looks in memory:

string plaintext = "hal"; // simplifiation of the input
int text_length = strlen(plaintext); // = 3

// Note this is pseudo code and just a visualization; -> is not used this way I'm just showing you what each element of the array is holding
plaintext[0] -> 'h' // first character
plaintext[1] -> 'a' // second character
plaintext[2] -> 'l' // third character
plaintext[3] -> plaintext[strlen] = 0; // fourth character NULL TERMINATOR
// Note that plaintext is 4 bytes in memory, though strlen is 3.
plaintext[strlen + 1] -> ????? // undefined memory 
// Note we should not try to look at this memory.  
// If on stack it could be other stack variables memory or a frame pointer etc
// If on heap it could be off the page entirely or another variables memory entirely... 
// Don't touch this memory.

If we rotate each character by 1 with your algorithm you should be able to see the following in ciphertext ( suggest you perhaps test it out )

int c_len = strlen(ciphertext); // print should show 3;
printf("%c\n", ciphertext[0]); // print 'i' and newline
printf("%c\n", ciphertext[1]); // print 'b' and newline
printf("%c\n", ciphertext[2]); // print 'm' and line
printf("%c\n", ciphertext[3]); // prints blankeline
printf("%s\n, plaintext); // prints "hal" and a new line.
printf("%s\n", ciphertext); // prints "ibm" and a new line.


You ignored some of the things I told you to change from the prior version of the question.

  1. This is now a critical flaw The length of the array for ciphertext is too small the size needs to be text_length + 1 to leave room for null terminator. As a result of not following this the next line where you set the null terminator is reaching into memory that isn't yours. This leads to undefined behavior and the program can do literally anything at this point. This is a common C mistake and is effectively a buffer overflow or stack overflow because you created this variable on the stack. Until you fix this you can't make any valid conclusions about the obvserable behavior of the rest of your program.

  2. Minor Get rid of j = text_length in the main body for loop it's not needed. Just use for (i = 0; i < text_length; i++ ) as the for loop definition (this is for the loop right after you set the null terminator.

  3. Minor You are calling rotate twice in the for loop body when you don't need to call it twice. Just call it once as you do properly in the second call to it.

  4. Major This is your one most key flaw aside from the memory issue in #1; Your print statement is completely wrong at the end, you are telling it to print a single character (%c) and then specifying the address of your null terminator of your string. This is wrong. The proper way to print what you need here is printf("%s\n", ciphertext); That's it. This is why your print is printing nothing but a newline; because you are telling it to print a null character. I apologize for not noticing this previously as this was present in your last code too

I strongly suggest you go back to review the lesson video or slides for this week and pay close attention to printf and information about strings.

Another note, don't just rely on check50 to be your testing. You should run the program yourself and make sure the output looks correct, and the next step is to use a debugger which I believe they cover in the next lesson. Until then I strongly suggest using additional print statements along the way that you remove or comment out before completion to provide yourself with 'debugging breadcrumbs' about the progress of your program.

Always include not just check50, but your own input and output of what you input into the program what you expect and what it does. This helps question answerers hone in and double check all loose ends.

  • Thank you again for the lengthy response and explanations! For point #1, I actually did change that and then changed it back after adding the null terminator, now realizing that it's still necessary. For points #2&3, got it. For point #4, I actually did try that at one point during all of this but it didn't work and still isn't working now when I use %s. I actually thought that made more sense and originally had it written like that but when it wasn't working I tried the %c since the ciphertext is declared as a char array.
    – AmyG
    Commented Jan 28 at 1:52
  • I hear what you are saying with checking the code myself with other inputs but I used the check50 as at least a starting point. It doesn't seem worth it to check other inputs until at least it can pass the check50. So even with all these changes, I'm still running into the same issue. Any idea what could be wrong?
    – AmyG
    Commented Jan 28 at 1:55
  • %c is wrong. that says print a character. %s is for printing a string. No you are 100% incorrect check50 shouldnt be your starting point.. it doesnt seem like you should expect to pass check50 if you cant pass basic intermediate points along the way. check50 is a final test. your starting point is enter key 1 and input 'a' and you should expect the output 'b'. if you cant pass that check50 isn't going to help you out.
    – UpAndAdam
    Commented Jan 28 at 17:58
  • put the code as you should and then we can figure out what wrong.. dont start another question. you already have 3.. just update this code in place and put a note that it has been updated. and then we can work with what you have. we can't keep debugging a moving target where you change things willy nilly just to try something. apply the suggestions and then let us know what is happening.
    – UpAndAdam
    Commented Jan 28 at 18:00
  • Please see edited question for the code as I have it with the suggested changes.
    – AmyG
    Commented Jan 28 at 23:54

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .