Caesar Cipher Program - Absurd Number in Array Output

Question

I'm actually writing about the same program as before, but I feel like I've made significant progress since the last time. I have a new question however; I have a function designed to store the frequencies of letters contained within the message inside an array so I can do some comparison checks later. When I ran a test segment through the function by outputting all of my array entries to see what their values are, it seems to be storing some absurd numbers. Here's the function of issue:

void calcFreq ( float found[] ) 
{
    char infname[15], alpha[27];
    char ch;
    float count = 0;
    FILE *fin;

    int i = 0;
    while (i < 26) {
        alpha[i] = 'A' + i++;
    }

    printf("Please input the name of the file you wish to scan:\n");
    scanf("%s", infname);

    fin = fopen ( infname, "r");
    while ( !feof(fin) ) {
        fscanf(fin, "%c", &ch);
        if ( isalpha(ch) ) {
            count += 1;
            i = 0;
            if ( islower(ch) ) { ch = toupper(ch); }
            while ( i < 26 ) {
                if ( ch == alpha[i] ) {
                    found[i]++;
                    i = 30;
                }
                i++;
            }
        }
    }
    fclose(fin);
    i = 0;
    while ( i < 26 ) {
        found[i] = found[i] / count;
    printf("%f\n", found[i]);
        i++;
    }
}

At like... found[5], I get this hugely absurd number stored in there. Is there anything you can see that I'm just overlooking? Also, some array values are 0 and I'm pretty certain that every character of the alphabet is being used at least once in the text files I'm using.

I feel like a moron - this program should be easy, but I keep overlooking simple mistakes that cost me a lot of time >.> Thank you so much for your help.

EDIT So... I set the entries to 0 of the frequency array and it seems to turn out okay - in a Linux environment. When I try to use an IDE from a Windows environment, the program does nothing and Windows crashes. What the heck?

Answer 1

alpha[i] = 'A' + i++;

This is undefined behavior in C. Anything can happen when you do this, including crashes. Read this link.

Generally I would advise you to replace your while loops with for loops, when the maximum number of iterations is already known. This makes the code easier to read and possibly faster as well.

Is there a reason you are using float for counter variables? That doesn't make sense.

'i = 30;' What is this supposed to mean? If your intention was to end the loop, use a break statement instead of some mysterious magic number. If your intention was something else, then your code isn't doing what you think it does.

You should include some error handling if the file was not found. fin = fopen(..) and then if(fin == NULL) handle errors. I would say this is the most likely cause of the crash.

Answer 2

Here are a few pointers besides the most important one of initializing found[], which was mentioned in other comments.

the alpha[] array complicates things, and you don't need it. See below for a modified file-read-loop that doesn't need the alpha[] array to count the letters in the file.

And strictly speaking, the expression you're using to initialize the alpha[] array:

alpha[i] = 'A' + i++;

has undefined behavior because you modify i as well as use it as an index in two different parts of the expression. The good news is that since you don't need alpha[] you can get rid of its initialization entirely.

The way you're checking for EOF is incorrect - it'll result in you acting on the last character in the file twice (since the fscanf() call that results in an EOF will not change the value of ch). feof() won't return true until after the read that occurs at the end of the file. Change your ch variable to an int type, and modify the loop that reads the file to something like:

// assumes that `ch` is declared as `int`

while ( (ch = fgetc(fin)) != EOF ) {
    if ( isalpha(ch) ) {
        count += 1;
        ch = toupper(ch);

        // the following line is technically non-portable, 
        //  but works for ASCII targets.
        // I assume this will work for you because the way you
        //  initialized the `alpha[]` array assumed that `A`..`Z`
        //  were consecutive.

        int index = ch - 'A';

        found[index] += 1;
    }
}

Answer 3

Check the definition of found[] in the caller function. You're probably running out of bounds.