Vigenere: Pset 2 | Code Prints Some Correct Characters, and Some Incorrect Characters

Question

I have been stuck on vigenere for two weeks now, and I think that I am almost there. My code prints out some correct characters, but also some incorrect ones. I think that it has something to do with my wraparound, but before I added one piece of code my wraparound worked fine.

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

    int letter;
    int counter = 0;
    char kk;
    char cc;

    int main(int argc, char* argv[]) {

        // Returns error if the command line arguments are not up to specifications
        if (argc < 2 || argc > 2) {

            // Returns value of error code
            return 1;

        // Else runs the cypher as usual
        } else {

            // Defines codeword and text to be coded
            char* code = argv[1];
            char* text = GetString();

            // Defines length variables
            int clength = strlen(code);
            int tlength = strlen(text);

            // Encrypts text
            for (int i = 0; i < tlength; i++) {

                // If the character is a space, ignores it
                if (text[i] == ' ') {
                    printf(" ");

                    // Else continues encrypting
                } else {

                    // Checks to see if the letter is alphabetical
                    if (isalpha(text[i])) {

                        // If the letter is capital
                        if (isupper(text[i])) {

                            // Determines what to add to the text
                            if (isupper(code[(counter % clength)])) {
                                kk = code[(counter % clength)] - 'A';
                            } else if(islower(code[(counter % clength)])) {
                                kk = code[(counter % clength)] - 'a';
                            }

                            // Writes out the letter
                            letter = (char)((text[i] + (char)kk) % 26);

                            // If the letter is lowercase
                        } else if(islower(text[i])) {

                            // Determines what to add to the text
                            if (isupper(code[(counter % clength)])) {
                                cc = code[(counter % clength)] - 'A';
                            } else if(islower(code[(counter % clength)])) {
                                cc = code[(counter % clength)] - 'a';
                            }

                            // Writes out the letter
                            letter = (char)((text[i] + (char)cc) % 26);

                            // Else if the letter is not either (a character)
                        } else {

                            // Letter is the same
                            letter = (text[i]);
                        }

                        // Increments counter
                        counter++;

                    // Else if the letter is not either (a character)
                    } else {

                        // Letter is the same
                        letter = text[i];
                    }

                // Prints out coded text
                printf("%c", (int)letter);

                }
            }

            // Prints new line
            printf("\n");

            // Returns no errors
            return 0;
        }
    }

Answer 1

Although it handles non-alphas, this code does not handle any letters correctly. It is encoding them as non-printable ASCII codes between 0 and 25. The formula for encoding letters is incorrect. You should review the class material on how to encode the letters. Most significant, the % modulo operator cannot be applied to the ASCII value of a letter.

While it probably isn't harming your code, you have some extraneous code, which harms readability and efficiency. For instance, there is code to specifically process spaces, and more code to process non-alphas. The non-alpha code should handle the spaces as well. Also, there are places where you cast variables where it is not needed. Most conspicuous is this: printf("%c", (int)letter); letter is created as an int, so this is totally unneeded. Further, letter is always being used to hold a char, so why not declare it as a char at the start?

You also have counter % clength in at least 8 different places. Wouldn't it be far simpler and more readable to do this one time where you increment counter and then simply use counter throughout. counter = (counter+1) % clength ;

The first else clause could also be removed and the remaining code can stand on it's own. Think about it. If the argc check fails, the program is going to terminate. If not, everything else is going to run. There's no need to enclose the whole of the remaining code in an else clause. It also introduces an opportunity for error if the program has to be edited later. Simpler is almost always better.

There are probably other things that you could do to simplify the code, but that should give you enough to chew on.

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