PSET2 Vigenere: Output bug, keyword loop going over strlen of key

Question

I am having an issue specifically with the second for loop (under "//iterate over key inputs" comment). The counter j is going over the strlen of argv[1], and thus feeding a wrong input to my shift function. So I am getting shifts greater than the ones corresponding to the key input (A, a = 0, B, b = 1, etc.).

I tried to fix this by placing a conditional in the loop, to check if j == m-1, then return j to 0. However this is not doing anything and I am not sure why.

Last issue is that the program is not printing non alphabetical characters in the ciphertext, even though I put a conditional for that function at the end of the code.

Thanks in advance.

int shift(char c);

    int main(int argc, string argv[])   
    {
        char c;
        int key;
        string p[1];

        if (argc != 2)
            //if input is not 2 strings, print the following and return 1 to end program
        {
            printf("Usage: ./vignere keyword\n");
            return 1;
        }
        else
        {
            int i = 0;
            int j = 0;
            int n = 0;
            int m = 0;
            if (isalpha(argv [1][j]) == false) 
            {
                printf("Usage: ./vignere keyword\n");
                return 1;
            }
            p[0] = get_string("plaintext: ");
            printf("ciphertext: ");

            //loop to iterate over each character of key input string
            for (i = 0, n = strlen(p[0]); i < n; i++)
            {                 
                //printf("i: %i", i);
                //check if plaintext input is a letter, if so, then...
                if (isalpha(p[0][i]))
                {
                    //iterate over key inputs
                    for (j = i, m = strlen(argv[1]); abs(j-i) < 1 ; j++)
                    {
                        //printf("j = %i", j);
                        //check if key input is alphabetic, if true, then...
                        if (isalpha(argv [1][j]))
                        {
                            c = argv [1][j];
                            //set int key as result of function shift
                            key = shift(argv[1][j]);
                            c = (p[0][i] + key);
                            //printf("k: %i", key);
                            printf("%c", c); 
                         }   
                        if (j == m-1)
                        {
                            j = 0;
                        }

                 }
                 if (isalpha(p[0][i]) == false)
                     //if plaintext is not a letter, print as is
                 {
                     printf("%c", p[0][j]);
                 }
             }

            }   
        }  
    }

    int shift(char c)
    {
        if (isupper(c))
        {
            return c - 65;
        }
        return c - 97;
    }

Answer 1

There are a number of problems with this code. Let's deal with your question first.

The code is constructed as a nested pair of loops. The outer for loop will loop over the length of the plain text to be encoded. The inner for loop will loop over the key. That means that for EACH character in the plain text, the entire key will be looped over. Why?

The key can't be looped across. The characters of the key need to be consumed one at a time, and only when an alpha char is processed from the plain text. This can't be done with a loop.

Other issues:
Let's talk about some other issues that I spotted.

string p[1]; declares an array with exactly 1 element. While it will work, this is not good form. For a single string, the following would be the better way to declare it: string p;.

At the beginning of the program is a block of code to check whether the parameter input is valid. if (argc !=2)... The if statement and related code block is fine. The problem is the else clause. The remaining code, essentially the entire program, is encapsulated in that else clause unnecessarily. If at some future date, the code were to need modification, whoever is altering the code may not realize that there is an else clause surrounding the code.

If the number of parameters is wrong, the if statement's code block will terminate the program. If not, the program can proceed with whatever code follows. There's no need to encapsulate the entire clause in an else clause. In fact, there's no need for an else clause. (An else clause is NOT required to follow an if statement.)

These issues have something in common. They will likely work, but they're bad practices that should be avoided. Hope this helps you program better. There may be other issues, but these are the ones that jump out at me.

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

Answer 2

I would recommend you to do this problem set in one for loop. You can just loop through the plaintext. Using nested loops will make the problem more complicates than you think. It's because you only need to loop through the plaintext and do changes to each character in the plaintext.

Some other advice with the code is, what if the plaintext is upper case, lower case, or space? They would shift a different direction due to the ASCII table.