Errors in Caesar pset2 and Wrong Output

Question

I am almost finished with Caesar only a couple of issues persist which I am unable to overcome

Here is my code -

#include <stdio.h>
#include <cs50.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>
int main (int argc, string argv[])
{
    int j;
    int n = strlen(argv[1]);
    string cyphertext[n];
    string s[n];
    if (argc == 2)
    {
    for (int i = 0; i < n ; i++)
    {
        if (isdigit(argv[1][i]))
    {
       string plaintext = get_string("plaintext: ");
       for (j = 0; j < n ; j++)
       if (isupper(plaintext[j]))
        {
            string cyptext[n];
            int letter = plaintext[j];
             
             printf("%c", (char)(65 + (letter + atoi((argv[1])))%26));
        }
       else if (islower(plaintext[j]))
        {
            string cyptext[n];
            int letter = plaintext[j];
        
            printf("%c", (char)(97 + (letter + atoi((argv[1])))%26));
        }
        else 
        {
            printf("%c", argv[1][j]);
        }
        return 0;
        }
        else
          {
        printf("Usage: ./caesar key");
        return 1;
          }
    }
    }
    else
    {
        printf("Usage: ./caesar key");
        return 1;
    }
}

The issues are all command line arguments work well however when I input only ./caesar without any following argument I get a Segmentation Fault. I do not get this error if I use more than needed argument or non digit arguments.

Secondly the output is not as expected however I can't find any apparent shortfalls in the code. Here is an example -

./caesar 4
plaintext: hi
e 

Please help.

Answer 1

You're getting a seg fault because the code tries to work with argv[1] before checking whether it exists. "Ready, Fire, Aim!"

The very first thing that must be done is to check the value of argc. If it is NOT 2, the program should terminate immediately. Period. It shouldn't be built on an if/else structure and shouldn't encapsulate any other code. It should simply check the value of argc.

Coding tip:

This code has a very common newbie error, a lesson that should be learned right at the start of any programmer's career. Well, actually two lessons.

Lesson 1: Program parameters should be validated at the very beginning of the code.

Lesson 2: Code should be compartmentalized and separated into small parts that do only one or two tasks at a time. Code should not be tightly coupled from one section to another, or intermixed without good reason.

In this case, the parameter validation is being spread out across the program. The code checks for the value of argc near the beginning, but if it is incorrect, the code takes action on it at the very end of the program. This is an invitation for code bugs and flaws to be added when the code is initially created or if it ever has to be modified in the future.

It also means that the entirety of the program code is encapsulated inside an if code block that should be separate and apart from the bulk of the program.

Simply put, the code should do this:

1. Check and validate the input parameters. If they are invalid, terminate the program.
2. Continue with the bulk of the program.

Steps 1 and 2 should have no significant connection.

The additional problems are also related to Lesson 2. This code tries to validate the key while it's trying to encode the text. It's just a jumbled mess. The code needs to compartmentalize these two tasks.

Task 1 is to validate the key. It should check that the key is all digits, convert the string in argv[1] into an actual number and store that number in a variable. This also means that the key is validated and stored as a number only once, not repeatedly. This also contains one task into one block of code with one mission, "validate and store the key".

Task 2 is encoding a letter. I'm just going to say that this is completely broken and you need to throw it out and try again. But let's go through what the code is actually doing:

        string cyptext[n];
        int letter = plaintext[j];             
        printf("%c", (char)(65 + (letter + atoi((argv[1])))%26));

First, why create a string called cryptest if it isn't used for anything? Second, instead of creating another intermediate var, letter, why not just use plaintext[j] in the print statement? If a var is only going to be used once, it's a red flag to ask "Is this var really necessary?"

Finally, the encoding. Say that the key is 5 and letter is 'B', ASCII value 66. Here's what happens. This is a valuable lesson and tool for debugging. You should learn how and be able to follow the code and work out what it does with pencil and paper, step by step.

// note the extra, unnecessary parentheses arouund argv[1]
(65 + (   letter + atoi((argv[1]))   )   %26)   
(65 + ('B' +  5)   % 26)
(65 + (66 +  5)   % 26)
(65 + 71 % 26)  //now, apply processing precedence rules
(65 + (71 % 26) )
(65 + (19) )
( 84 )
'T'

The correct result should be 'G'.

A review of the class material on encoding the letters would be very valuable now.

Finally, a last line feed is missing.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)