0

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;
    }
1
  • I fixed everything in the end, made the code into one loop using two counters. Made p into a single string rather than array, and removed else conditional. Finally submitted :) Jul 1, 2019 at 18:10

2 Answers 2

2

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. ;-)

3
  • About the nested loops, I had the problem you mentioned, where the first loop would only go through one character of the plaintext to be encoded, but go over all the chars in the key. I fixed this with the condition in the key loop, so that it only goes through one character of the key and then stop the loop, therefore going back to the outer loop. I'm not sure if that's the best practice but I am not sure how else to do it. Jul 1, 2019 at 15:02
  • About the string array, I chose to do that so that I could select individual characters from it because that's how they instruct you do it with the argv[1][i]. Can you still choose the individual characters from a string by simply using the brackets as well? Jul 1, 2019 at 15:05
  • Thanks for the comment about the else clause, will fix that Jul 1, 2019 at 15:06
1

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.

2
  • I chose to do the nested loops cause the instructions say to use a different counter for the key, so I assumed I would need two loops. How would you suggest to do it within a single loop? I got lost in that. About the upper/lower case, the function at the bottom takes care of that, as it deducts 65 and 97 from the ascii value respectively, essentially always setting a = 0 and A= 0, b = 1, B=1, etc. With the spaces when I run the program it doesn't seem to register them, I need to fix that but not sure how. It does the same with non-alphabetical characters even though I have a clause for it Jul 1, 2019 at 15:10
  • Some pseudo code would help you in this case. In a single loop, you only need to loop through plaintext. Set i = 0 i < plaintext, in this plain text, you will need to use if-else if chains in the loop. Recall string is just characters of arrays, you will change every character by determining whether this is a space/punctuation, lower case, or upper case.
    – Jn Wu
    Jul 1, 2019 at 20:31

You must log in to answer this question.

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