PSET 2 Caesar : help debugging caesar : continued #2

Question

I'm looking for some (more) help trying to figure out caesar. I finally got it to compile but it's not working properly. When I run check50, it looks like there are two main issues -

one, it's not correctly checking if every character of the key is a digit (the sad face is on "handles non-numeric key - timed out while waiting for program to exit"). I've been wracking my brain over the only_digits function and how to fix it but can't figure it out.

and the second thing is that while the program is correctly shifting each plaintext, it's not printing out the result.

Any help greatly appreciated!

original question: help with Caesar problem from Pset2

#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
    only_digits(argv[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];

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

    // print out the ciphertext
    printf("ciphertext: %c\n", ciphertext[text_length]);
    return 0;

}

bool only_digits(string s)
{
    for (int i = 0, j = strlen(s); i < j; i++)
    {
        if isdigit(s[i])
        {
            return 0;
        }
        else
        {
            printf("Usage: ./caesar key\n");
            return 1;
        }
    }
    return 0;
}

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 - 65) + n) % 26;
        // return the result
        c = rotated_char + 65;
    }
    else if (islower(c))
    {
        rotated_char = ((c - 97) + n) % 26;
        c = rotated_char + 97;
    }
    else
    {
        rotated_char = c;
    }
    return rotated_char;
}

Answer 1

Overall the common theme is that you need to remember to do something with the return values from your function calls that return a value.

I'm mostly not going to spell out exact solutions, but leave you with hints that should enable you to solve it. Ask follow ups in comment if you need more.

1. Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

2. You need to do something with the returned result from your call to only_digits.

   • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
   • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
   • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.

3. Minor Issue

   • You have an issue with the size you use for ciphertext; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at ciphertext[text_length] either before or after your population of ciphertext. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you. UPDATE to set the null terminator you just have to say ciphertext[text_length] = 0; or ciphertext[text_length] = '\0';

4. Problem with your construction of ciphertext

• Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plaintext. As a result in the next line when you say ciphertext[i] = plaintext[i] effectively nothing happens, it just copies over the original value of plaintext[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?

5. Side Notes
   • I would suggest not declaring ciphertext the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
   • No reason to create variable j, just use text_length
   • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.