Program based on Vigenère’s cipher

Question

I would like to get some helpful thoughts on this and if there is a way to break it. Is there any point to not use fgets in that form?

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

#define GETNAME_SIZE 100

char *getPhrase() {

    int size;
    char phrase[GETNAME_SIZE];
    char *phraseHolder;
    printf("Enter a phrase:");

    fflush(stdout);

    if (fgets(phrase,sizeof phrase,stdin) == NULL ) {
        return NULL;
    }

    size = strlen(phrase)+1;
    phraseHolder = malloc(size);
    if (phraseHolder == NULL) {
        return NULL;
    }
    strcpy(phraseHolder,phrase);
    phraseHolder[strcspn(phraseHolder,"\n")] = '\0';
    printf("The phrase you introduced is: <%s>\n", phraseHolder);

    return phraseHolder;

}

char *getChangedPhrase(char *phrase) {

    int size,i,codeSize;
    int j = 0;
    char *code;
    int pace;
    size = strlen(phrase)+1;
    if(size <2) {
        printf("Enter a bigger phrase!");
        return NULL;
    }


    code = malloc(size);
    if (code == NULL) {
        return NULL;
    }
    fflush(stdout);
    printf("Enter the code that you want to be used with your phrase!(only letters)\n");

    if (fgets(code,size,stdin) == NULL) {
        return NULL;
    }
    code[strcspn(code,"\n")] = '\0';
    codeSize = strlen(code);
    for(i = 0 ; i < size ; i++) {
            if(phrase[i] < 65 || (90 < phrase[i] && phrase[i] < 97) || 122<phrase[i] ) {
                continue;
            }
            else {
                while(code[j] < 65 || (code[j] > 90 && code[j] < 97) || code[j] > 122 ) {
                    j++;
            }
                pace = code[j];

            if(code[j] > 96 && code[j] < 123) {
            pace = pace - 97;

            if(phrase[i] + pace > 122 )
                phrase[i] = phrase[i] + pace - 26;
            else
                if( phrase[i] + pace > 90 && code[j] < 90)
                phrase[i] = phrase[i] + pace - 26;
            else
                if(phrase[i] + pace > 90 && code[j] > 96 && phrase[i] < 91 ) {
                   phrase[i] = phrase[i] + pace - 26;
                }
            else
                phrase[i] = phrase[i] + pace;


        }
            else if ( code[j] > 64 && code[j] <91 ) {
                pace = pace - 65;

                if((phrase[i] < 91 && (phrase[i] + pace) > 90) || (phrase[i] + pace) > 122 )
                    phrase[i] = (phrase[i] + pace - 26);

                else
                    phrase[i] = phrase[i] + pace;
            }

            j++;

            if(j == codeSize) {
                j = 0;
            }
        }

    }
    free(code);
    phrase[strcspn(phrase,"\n")] = '\0';
    return phrase;
}


int main() {
    char *phrase = getPhrase();
    char *modifiedPhrase = getChangedPhrase(phrase);
    printf("The changed phrase is: \n <%s> \n", modifiedPhrase);
    //free(phrase);
    printf("Press ENTER key to Continue\n");
    getchar();
    getchar();
}

Answer 1

Your code is pretty clean and well-formed, but I do have several critiques.

Unexpected behavior

Your program will exhibit unexpected behavior if the phrase entered by the user is longer than GETNAME_SIZE - 2 chars. In that cases, the fgets() call in getPhrase() will leave the tail of the line unread, and reading should be expected to pick up there in the subsequent getChangedPhrase() call (to terminate no later than the end of that line). The user will no doubt expect the phrase to be encoded according to the next line they enter, not the tail of the first.

To fix that, you should either arrange in getPhrase() to keep reading through the end of the line. You can discard characters after the GETNAME_SIZE - 1^th if you like, or you can arrange to append them to the phrase (you'll need realloc()). Just don't leave a tail of the line unread.

getChangedPhrase() behaves similarly when it reads the key, which again could be surprising. It is not certain to be sufficient to limit the input key length to the message length because you don't necessarily use all of the key characters (you skip those that aren't ASCII letters). Cutting off the tail of the key can therefore change the result.

This fragment looks wrong:

                if( phrase[i] + pace > 90 && code[j] < 90)
                phrase[i] = phrase[i] + pace - 26;

I think you mean

                if( phrase[i] + pace > 90 && phrase[i] < 91)
                phrase[i] = phrase[i] + pace - 26;

See also below.

Flawed implementation

Your code can easily be made to overrun the bounds of the code array. In fact, that will happen if the number of usable key characters is smaller than the number of letters in the message. To (partially) fix this, you need to wrap around the end of the key inside the while loop, rather than at the end of the for loop where you now do that.

That will, however, expose another flaw in your implementation: it does not handle the case where there are zero usable key characters. Your current implementation will just immediately run off the end of the array in that case; after the fix, it will loop infinitely. See below for a suggestion that will help.

Error handling

getPhrase() does a decent job of noticing when memory allocation fails or nothing is read from the input. It helpfully returns NULL in those cases. main(), however, ignores those signals and unconditionally passes the result to getChangedPhrase(). For its part, getChangedPhrase() assumes that its argument is always valid. Either main()'s behavior or getChangedPhrase()'s could be ok in isolation, but together they constitute a flaw producing undefined behavior under some circumstances.

Similarly, getChangedPhrase() returns NULL in some failure conditions, but main() pays no attention, assuming that the return value is a valid pointer, in attempting to print it.

Implementation improvement

If you have strdup() (which is standardized by POSIX, not C itself) then it is a convenient alternative to strlen() + malloc() + strcpy().

getChangedPhrase() calls fflush(stdout); before prompting for the encoding key. This is useless. Move the flush after the prompt, or remove it altogether. Since the prompt ends with a newline, it is probably unnecessary to flush even after printing the prompt.

I remarked above on problems with your key handling. I observe also that you handle your key inefficiently if it is short relative to the phrase to encode. You shouldn't need to check on each pass through the key which characters are usable. You shouldn't need to account for upper- vs. lowercase on each pass, either, if indeed you make them behave the same way. Instead, process the key once, before the encoding loop: remove characters other than letters, and convert all the letters to a standard case. Then it's also easy to validate result to ensure that there is at least one (or two, or whatever) key characters.

It doesn't make sense to me that your encoding code is not parallel for upper- and lowercase key characters. The code for uppercase keys looks equally applicable to lowercase keys except the computation of pace, but you use a more complicated and difficult to follow scheme for lowercase key characters. Indeed, one of the alternatives for lowercase keys cannot even be exercised, because code[j] < 90 and code[j] > 96 cannot be true at the same time.

Design improvement

Your function getChangedPhrase() modifies the input string in-place, but also returns a pointer to it. On the other hand, the function's name suggests that it will create a new phrase. I suggest changing that function's signature to

void encodePhrase(char *);

to clarify what it's doing. If you wish to have the ability to signal an error, then you could consider using an int return value to signal the status, following the model of many standard library functions.