1

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;
}
0

1 Answer 1

0

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?
  1. 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.
7
  • Thank you so much for your thoughtful and lengthy response!! I'm really stuck with the only_digits function. I understand logically the problem (I think), but I don't know how to tell the loop to go again until it reaches the end of the string. In other words, after if (isdigit(s[i])), I don't know what to put inside the following curly braces in order to continue the loop. I tried creating a variable, first as an int, then as a string, to "store" the value of s[i] for as long as the loop continues but that didn't work. I tried using a different kind of loop (do, while) but still stuck..
    – AmyG
    Jan 25 at 13:29
  • nothing.. the loop will autmatically go on just like in your other for loops in main. at the end of the body of the loop it will perform the provided increment instruction, check the condition and loop again. your condition for the loop is correct you just have remove the incorrect early return true that you do inside the loop body and put it outside the loop to be done when the loop completes and didnt return early for a false case... if my anwer is useful please upvote it and consider accepting it if it solves your problem
    – UpAndAdam
    Jan 25 at 17:33
  • ok, I realize I was making it more complicated than I needed to!! I fixed only_digits and most of the other issues you mentioned in your comments but I'm still getting stuck on how to carry the info from the rotate function back to ciphertext. I added a line of code plaintext[i] = rotate(plaintext[i], key); between rotate(plaintext[i], key); and ciphertext[i] = plaintext[i]; to try and fix this but it's still not working. Any further advice you can offer me here?
    – AmyG
    Jan 25 at 20:25
  • One more thing - I understand what the null terminator is but I'm not sure what exactly it means to "set" it. This was covered only briefly in the lecture and section videos and looking through my notes, I don't see a way to declare it somehow, other than to make sure the array has an extra character at the end to leave space for it. Can you let me know what other steps I need to take?
    – AmyG
    Jan 26 at 2:00
  • @AmyG I'll add null terminator instructions to my answer.. for your other question thats perplexing... it should work... however I wouldn't alter plaintext at all just assign the result of the rotation to ciphertext. ciphertext[i] = rotate(plaintext[i], key); that's it. Otherwise what did you need a seperate string ciphertext for if you are going to modify plaintext? For null terminator: ciphertext[text_length] = 0; or ciphertext[text_length] = '\0'; Again PLEASE UPVOTE answer if it is helpful and accept it if it answers your question. That's all we ask in return for helping.
    – UpAndAdam
    Jan 26 at 17:08

You must log in to answer this question.

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