2

I'm following along with cs50x and in problem set 2. This is the idea I had for solving the Caesar problem. I'm yet to implement the key idea due to the fact that it won't print out the word. I'm new to arrays and have searched a bit about why this is occurring. I think that I'm overcomplicating the code and could just use the string given by the user instead of transferring it to a function but now that I've started the idea I want to know why it isn't working and if there is a way to make it work. When ran, the program should accept a command line of a single number, if it has no command line it should fail, if the number is negative it should fail, if it is not a number it should fail and if it has more than 1 argument it should fail. Thanks

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



string cipher(string word, int key);

int main(int argc, string argv[])
{

    // Checks whether the user inputted only 1 argument
    if (argc == 2)
    {
        // Convert argv to an int
        int key = atoi(argv[1]);
        string plainText = get_string("plaintext:  ");

        // Use function to return the (soon to be encrypted) string
        string cipherText = cipher(plainText, key);

        // Print for how long the word is
        int n = strlen(plainText);
        for (int i = 0; i < n; i++)
        {
            // Print the char of the array based upon the iteration of the loop which runs for however long the word is
            printf("%c", cipherText[i]);
        }
        printf("\n");
        // If input is not a positive integer then it will fail
        if (key < 1)
        {
            printf("Usage: ./caesar key\n");
        }
    }
    else
    {
        // If user inputted too many or no inputs then it will fail
        printf("Usage: ./caesar key\n");
    }
    return 0;
}

string cipher(string word, int key)
{
    // Find the length of the word in order to set the size of the array
    // This is so that both strings, the word inputted and the word to return are the same to add to
    int n = strlen(word);

    string cipherText[n];

    // Loop through the input word
    for (int i = 0; i < n; i++)
    {
        // If char[i] is a letter then copy that letter into ciphertext
        if (isalpha(word[i]))
        {
            cipherText[i] =& word[i];
        }
        else
        {
            cipherText[i] =& word[i];
        }
    }
    // Return the array which, for example the input word is nobody
    // Return array[n, o, b, o, d, y]
    return cipherText[0-n];
}
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Angus Hay
  • 23
  • 3
  • 2
    `cipherText[0-n]` What is the intention there? `0-n` is a negative index. – kaylum Sep 08 '22 at 01:16
  • Furthermore does your compiler not give you any warnings? `cipherText[i]` is a `string` whereas `word[i]` is a single char. So `cipherText[i] =& word[i];` does not make sense. Not to mention that `cipherText[i]` is never initialised. Really not sure what you are trying to do. – kaylum Sep 08 '22 at 01:18
  • @kaylum I was trying to access the values of the array from 0 through n so if n was 4 then access cipherText[0, 1, 2, 3 ,4] – Angus Hay Sep 08 '22 at 01:21
  • @kaylumThe compiler I think gives a warning if I use 0-n however i just used n to try and access at least one of the values. the =& was a result of the compiler warning and I added it to allow make to work. I thought that strings are arrays of characters and so I could access the letters individually and copy them over. thanks – Angus Hay Sep 08 '22 at 01:24
  • This looks like "shotgun programming"... blast away hoping for something that seems to fulfill the need. The first step is good - checking for exactly 2 program arguments. Then, convert the string at `argv[1]` to an integer value. Unfortunately, the value is USED before it is tested as being <1 (more obvious might be "key <= 0")... Step by step coding (to a plan) testing every incremental addition/modification will get you where you want to be... Don't "shotgun" and hope for success... – Fe2O3 Sep 08 '22 at 02:13
  • @Fe2O3 I don't understand, should the if statement be further up? Yes <= does sound better so I'll change that. I feel like I did test each addition? how is that different to shotgunning, I outlined a plan and made some code and then went from there? thanks – Angus Hay Sep 08 '22 at 02:20
  • Further up? Yes. Immediately after the call to `atoi()`... If the user typed "-27", then give an error message and stop execution. For development, add `printf( )` after the `atoi( )` that (temporarily) shows you the integer value of "key". Every operation you code, especially as a beginner, should be followed by some confirmation to you that the operation does what you expected. Inside `cipher()` print the value of 'n' to confirm that the strlen is as expected. You can remove these 'aids' later, but they serve to help you move forward reliably. Tedious? Certainly, but quicker than debugging. – Fe2O3 Sep 08 '22 at 02:27
  • Okie coke, thanks for catching that I did throw it in because I figured I had it done correctly but hadn't tested it since. Thank you! – Angus Hay Sep 08 '22 at 02:34

2 Answers2

1

The issue is that you are attempting to copy the address of the "word" character array characters into the associated cipher text array element which will print out unknown characters (noted in the above comments).

    // Loop through the input word
    for (int i = 0; i < n; i++)
    {
        // If char[i] is a letter then copy that letter into ciphertext
        if (isalpha(word[i]))
        {
            cipherText[i] = &word[i];
        }
        else
        {
            cipherText[i] = &word[i];
        }
    }

When I ran your program with the code like that, I indeed got a series of question marks.

@Una:~/C_Programs/Console/CypherCS50/bin/Release$ ./CypherCS50 12
plaintext: Hello
?????

I then revised it to perform a copy of character elements from "word" to "cipherText".

// Loop through the input word
for (int i = 0; i < n; i++)
{
    // If char[i] is a letter then copy that letter into ciphertext
    if (isalpha(word[i]))
    {
        cipherText[i] = word[i];
    }
    else
    {
        cipherText[i] = word[i];
    }
}

Then, reran the program.

@Una:~/C_Programs/Console/CypherCS50/bin/Release$ ./CypherCS50 12
plaintext: Hello
Hello

Seeing that the same data came out, my guess is that you still need to work on the actual encryption bits. But, the crux of the issue was referencing the memory of the work array elements.

Give that a try.

NoDakker
  • 3,390
  • 1
  • 10
  • 11
  • Hi thank you! What do you mean referencing the memory of the work array? what is the work array? Referencing? I'm still very noob to this so I don't quite understand – Angus Hay Sep 08 '22 at 01:45
  • Sorry. I mistyped. I meant "word" not "work". – NoDakker Sep 08 '22 at 01:45
  • I've swapped the string cipherText to a char cipherText however I still don't understand how to return the array from the 0 index to the nth index? How did you do that I can't see it on your reply – Angus Hay Sep 08 '22 at 01:46
  • Ah, no problem I figured but wanted to make sure lol – Angus Hay Sep 08 '22 at 01:46
  • I actually had to make the cipherText variable global, because when I attempted to return the variable as defined within the function, the value goes out of scope (it is actually destroyed). Also, the compiler issued a warning about that. – NoDakker Sep 08 '22 at 01:52
  • How do you make a variable global? I think I tried putting it in main. I also tried putting it alongside the function prototype however now N is undefined and I can't then define n without having gotten a string etc – Angus Hay Sep 08 '22 at 01:55
  • You would place your definition at the top of the program after the "#include" statements but before the function definitions. In my case, since I don't have the CS50 libraries installed, I substituted a character pointer definition for the CS50 "string" definition (char cipherText[64];). Having said that, I opted for the global variable so that I did not have to do a whole bunch of revising of the code. – NoDakker Sep 08 '22 at 02:00
  • Thank you! It worked, I understand better now – Angus Hay Sep 08 '22 at 02:05
  • You're welcome. Good luck on your coding journey. – NoDakker Sep 08 '22 at 02:08
0

This does not fix your OP issue, but addresses another issue and responds to our exchange in comments above. Here is a "skeleton" demonstrating how you might approach incrementally developing code for this task. The 'excessive' printf's serve to prove that things are proceeding as you want as the source code becomes more elaborate..

// Functions defined before use do not need to be prototyped
// do-nothing "skeleton" to be completed
string cipher(string word, int key)
{
    printf( "In cipher with key %d and str '%s'\n", key, word ); // temp code confirmation

    return word; // for now...
}

int main(int argc, string argv[])
{
    if (argc != 2)
    {
        printf("Usage: ./caesar key\n");
        return -1; // early termination
    }
    printf( "argv[1] = %s\n", argv[1] ); // prove functionality

    int key = atoi(argv[1]);
    printf( "key = %d\n", key ); // prove functionality

    if (key <= 0)
    {
        printf("Key must be positive integer");
        return -1; // early termination
    }
    string plainText = get_string("plaintext:  ");
    printf( "plain = %s\n", plainText ); // prove functionality

    string cipherText = cipher(plainText, key);
    printf( "cipher = %s\n", cipherText ); // prove functionality

    return 0; // All done!
}
Fe2O3
  • 6,077
  • 2
  • 4
  • 20