PSET 2 Caesar : help part 3 : ciphertext rotating correctly but not printing out

Question

*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

#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);
        }
        else
        {
            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';
    }
    else
    {
        rotated_char = c;
    }
    return rotated_char;
}

Answer 1

Updated
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.

Original

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.