0

I'm having some issues with this problem set from CS50. What I'm trying to do is encrypt a message and it doesn't print the result.

This is what debug50 shows ciphertext to be before it reaches printf

And this is after

This is my code, it's a mess

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

bool check_key(char k);
string cipher (char text[], int key);

char alphabet[] = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};


int main(int argc, string argv[])
{
    // Check the key
      // Check if it's more or less than 1 command line argument
        // If not print ERROR
    if (argc != 2)
    {
        printf("ERROR!\n");
        return 1;
    }
      // Check if it's non negative - <= 0
      // Check if it's a valid int
        // If not print "Usage: ./caesar key"
    char k = *argv[1];
    if (check_key(k) == 0 || k == '0')
    {
        printf("Usage: ./caesar key\n");
        return 1;
    }
    // Turn key into and int
    int key = atoi(argv[1]);
    // Get text
    string plaintext = "ABC"; //I used this as a placeholder for get_string("plaintext:  ");
    string ciphertext = cipher(plaintext, key);
    // Print cipher
    printf("ciphertext: %s\n", ciphertext);
}

bool check_key(char k)
{
    return isdigit(k); // Doesn't take negative values
}

string cipher (char text[], int key)
{
    // Take plaintext characters and change each character of it with the character key positions away
        // Add the character from alphabet array to another chiper array
        // Add cipher array to cipher string and return
    int position = 0;
    int text_position = 0;
    int text_length = 0;
    char ciphertext[strlen(text)];
    do
    {
        if (alphabet[position] == text[text_position])
        {
            int key_position = position + key;
            ciphertext[text_position] = alphabet[key_position];
            position = 0;
            text_position++;
        }
        else
        {
            position++;
        }
    }
    while (text[text_position] != '\0');
    string final_cipher = ciphertext;
    return final_cipher;
}

I can't get it to print the ciphertext. I was wondering if the problem is because of the way I turned the array into a string, but I don't know any other way to do that besides using the overloaded operator.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
cyberbully
  • 13
  • 1
  • 1
    In `cipher()` in the `do/while`, if `text_position++;` is not reached you have an endless loop. – 001 Aug 22 '22 at 19:26
  • Is this C or C++? – Jardel Lucca Aug 22 '22 at 19:28
  • 1
    Assuming you have a type-alias for `string` (being a `char *`) then you have *two* problems: The first is that your `cipher` function return a pointer to a local variable (a variables whose life-time ends when the function returns, and ceases to exist); The second problem is that you never terminate `ciphertext` as a string. Both of these problems each lead to *undefined behavior*. – Some programmer dude Aug 22 '22 at 19:30
  • I’m using C# for this – cyberbully Aug 22 '22 at 19:34
  • @cyberbully _"I’m using C# for this"_ - Then why did you tag it with C? – Ted Lyngmo Aug 22 '22 at 19:38
  • 1
    @SteveSummit I know, but I don't see that header-file where it's defined (which makes the shown code *not* a [mre]). – Some programmer dude Aug 22 '22 at 19:40
  • 2
    @cyberbully That's most definitely isn't C#! You should probably stop using CS50, it's not in high regard as a teaching resource among experienced programmers. It hides details that make it hard to actually learn what's happening or what's going on, and it seems to teach more "problem-solving" with problems that aren't for beginners or aren't useful outside of all the common "competition" or "judge" sites that have cropped up over the past couple of years. – Some programmer dude Aug 22 '22 at 19:41
  • @Someprogrammerdude what course would you recommend? – cyberbully Aug 22 '22 at 19:44
  • Online? There seems to be very little available that appears to be useful. The best tip is to stay away from any site that caters to the "competition" or "judge" crowd, as they are most definitely not any kind of learning or teaching resource. There seems to be plenty of Youtube videos of dubious quality around, so it's hard to find the good ones there. The best way IMO is to invest in books. A couple of beginners books to start with, get the feel of the language and its constructs, and then work from there with deeper learning about what tickles you or seems interesting. – Some programmer dude Aug 22 '22 at 19:48
  • 1
    I can personally recommend https://www.eskimo.com/~scs/cclass/cclass.html – Steve Summit Aug 22 '22 at 19:49
  • Thanks, I’ll research some books and get started – cyberbully Aug 22 '22 at 19:53
  • [Here's a list of books](https://stackoverflow.com/questions/562303/the-definitive-c-book-guide-and-list). It's not really curated or all books updated to their latest editions, but it's a start. The good news is that the core of C haven't changed that much in over 20 years, so many older books can still be used. – Some programmer dude Aug 22 '22 at 19:53
  • As for how to easily solve your main problem you're asking about: Create the `ciphertext` array in the `main` function, bit enough to fit all the text you need *plus* the terminator. Then pass it as an argument to the `cipher` function. Then the`cipher` function doesn't have to return anything. – Some programmer dude Aug 22 '22 at 19:55
  • 1
    @AndreasWenzel CS50's valiant attempt to *not* teach newbies about `scanf` is salutary indeed. – Steve Summit Aug 22 '22 at 20:40
  • @Someprogrammerdude: In my now-deleted comment in which I replied to your comment, I claimed that in week 2 of CS50, the true nature of strings is revealed to the students. Since the problem OP is working on is only supposed to be solved after watching week 2, I claimed that your criticism against CS50 wasn't justified, because OP was supposed to be aware of the true nature of strings after week 2. [continued in next comment] – Andreas Wenzel Aug 23 '22 at 09:58
  • @Someprogrammerdude: [continued from previous comment] However, after looking more closely at CS50, I noticed that only in week 4 (which OP has probably not yet reached) is it revealed to the students that a `string` is a `typedef` for a `char *`. That is because pointers are only explained in week 4. What is revealed in week 2 is only that a `string` is a null-terminated character array. Therefore, my statements in this regard were partially incorrect. For this reason, I have deleted my comments. – Andreas Wenzel Aug 23 '22 at 09:58

2 Answers2

0

you need to allocate a place for ciphertext array with malloc and make your funktion return a char*, or if you have a String struct allocate for this a place and allocate for the char* also.

Jabal
  • 21
  • 2
0

In the function cipher, in the line

char ciphertext[strlen(text)];

you are creating a local character array. The lifetime of this array will end as soon as the function cipher ends.

In the line

string final_cipher = ciphertext;

you are not creating a copy of the array. Instead, you are creating a reference to the existing array. This is because the data type string does not actually contain a string. It merely contains a reference to a character array, which may contain a null-terminated string. In week 4 of CS50, you will learn that the data type string is actually an alias for char *, which is a pointer, i.e. a reference to another variable or array.

Therefore, in the line

return final_cipher;

you are not returning a copy of the array, but you are merely returning a reference to the existing array ciphertext.

However, as previously stated, the referenced array ciphertext will cease to exist as soon as the function returns. Any attempt to use this reference afterwards will lead to undefined behavior.

Since the array ciphertext no longer exists, the program will probably use the memory previously used by by the array for other purposes. That is probably why you are seeing the memory's value changing unexpectedly in the debugger. The function printf is probably using that memory for something.

There are several solutions to this problem:

  1. You could create a proper copy of the array ciphertext with a lifetime that does not end when the function ends, so that the function cipher can return the copy back to main. This would require you to use the function strcpy and maybe also malloc. However, you will probably not understand how to use any of these functions until you have completed week 4 of CS50, so I do not recommend this solution for now.

  2. Instead of attempting to store the individual characters of the ciphertext, you could print the individual characters immediately and then forget about them. This is probably the solution intended by the CS50 authors.

Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39
  • Thank you so much! I ended up using malloc and strcpy and worked like a charm! – cyberbully Aug 26 '22 at 11:20
  • @cyberbully: I'm pleased I was able to help. Note that some [platforms](https://en.wikipedia.org/wiki/Computing_platform), such as Linux and Windows, support a function called [`strdup`](https://manual.cs50.io/3/strdup) which combines `malloc` and `strcpy` into one function. If your platform supports it, it would be easier to use that function instead. Some other platforms may not yet support this function, but this is likely to change, as that function is likely to get included into the upcoming ISO C23 standard. – Andreas Wenzel Aug 26 '22 at 11:23