1

I have a program which reads floating point numbers from a .txt file and puts them into an array but I have a problem with calculating median. Everything is working perfectly except from this. What am I doing wrong?

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


int compare (const void * a, const void * b)
{
  float fa = *(float*) a;
  float fb = *(float*) b;
  return (fa > fb) - (fa < fb);
}



//median calculations//
float median1(float[],int); 
float median1(float array[],int n) 
{  
qsort(array, n, sizeof(float), compare);

if(n%2==0)  
    return (array[n/2]+array[n/2-1])/2;  
else  
    return array[n/2];  
}



float x,l=~(257<<23),a,s,t,median;
main(int n,char**f)

{
char fname[20];
int i;
a=-l;

printf("Please type the file name with an extension (.txt)\n");
scanf("%s", fname);


f=fopen(fname,"r");
for(n=0;fscanf(f,"%f",&x)>0;n++,s+=x,x<l?l=x:0,x>a?a=x:0,t+=x*x);

float array[n];

fseek (f, 0, SEEK_SET);

for (i=0; i<n; i++) 
        {
            fscanf (f, "%f", &(array[n]));
        }

median=median1(array,n);  

printf("Sample size = %d\n", n);
printf("Minimum = %f\n", l);
printf("Maximum = %f\n", a);
printf("Mean = %f\n", s/n);
printf("Median = %f\n",median);
printf("Standard deviation = %f\n", sqrtf(t/n));

return 0;
} 
xLokos
  • 69
  • 1
  • 9
  • Could you describe what's not working. Is it giving the wrong value? Try to step through it with a debugger. – MicroVirus Jan 26 '14 at 02:17
  • @Nabla what do you mean twice? – xLokos Jan 26 '14 at 02:19
  • 3
    Why would you have a collection of global variables like `x`, `l`, etc? Make them local. I'm puzzled about the initializer for `l` — quite honestly, I've no idea what it's supposed to do with bit operations on an integer assigned to a `float`. You'd also lose any marks I was giving out for the line `for(n=0;fscanf(f,"%f",&x)>0;n++,s+=x,xa?a=x:0,t+=x*x);`. That is diabolical — ridiculous — code. – Jonathan Leffler Jan 26 '14 at 02:20
  • This is the highest possible float value. – xLokos Jan 26 '14 at 02:32
  • @xLokos The highest possible float value is `FLT_MAX` from ``. –  Jan 26 '14 at 02:34
  • But it's still high enough I think? It's about 011111110111111111111111111111111111 in binary code. – xLokos Jan 26 '14 at 02:37
  • 1
    @xLokos It's high enough so long as no one tries to input something higher. –  Jan 26 '14 at 02:41
  • @JonathanLeffler The program in this question is a (barely) indented version of a humorous answer given to the OP's previous question. The intention behind `~(257<<23)` is clearly to create the bit pattern for FLT_MAX, except that 1) conversion from `float` to `int` does not work this way 2) `257 << 23` is undefined behavior on typical 32-bit platforms. – Pascal Cuoq Jan 26 '14 at 09:21
  • @PascalCuoq: Ahah — much is explained! – Jonathan Leffler Jan 26 '14 at 15:08

2 Answers2

4
fscanf (f, "%f", &(array[n]));

should be

fscanf (f, "%f", &(array[i]));

You are only writing to one array element and that one is out-of-bound.

Even if this wouldn't result in undefined behavior, you would still work with garbage values later on.

See @JonathanLeffler's comment for some further remarks on your code.

  • The first read counts how many values there are. This is used to allocate the VLA `float array[n];` of the correct size. – Jonathan Leffler Jan 26 '14 at 02:21
  • @JonathanLeffler Ah ok, I was confused because `n` is for some reason the first parameter to `main`. –  Jan 26 '14 at 02:23
  • You're excused for being confused; I hadn't even noticed that. Gruesome doesn't really begin to describe some of the coding feats in here. Notice that `f` is the other argument to `main(int n, char **f)`, but is also used as a `FILE *` in `f = fopen(fname, "r");` — there's a serious case of not paying attention to compiler warnings here! – Jonathan Leffler Jan 26 '14 at 02:25
  • Yup, what he said. I need to create an array to calculate median, and I didn't want to use dynamic one. – xLokos Jan 26 '14 at 02:25
  • Reading the file twice is probably more costly than using dynamic memory allocation. You'd have to measure to be sure, and if the file is small enough (say under 1 KiB), then you probably won't be able to detect the difference. – Jonathan Leffler Jan 26 '14 at 02:27
  • If it's not an error I don't even look at it ;) Still after changing that line it returns value wchich isn't even in the txt file – xLokos Jan 26 '14 at 02:28
  • @xLokos: You need to learn to write code so that it is squeaky clean with no warnings even with the warning levels turned way up. If you use `gcc`, the bare minimum should be `-Wall`. I don't write code that doesn't compile clean with `gcc -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes -Wold-style-definition -Werror -O3 -g -std=c11` as my default warnings. I sometimes add some others; very occasionally I relax the prototype warnings if it is someone else's code. – Jonathan Leffler Jan 26 '14 at 02:31
  • So what should I change the `main` arguments to? – xLokos Jan 26 '14 at 02:34
  • 1
    @xLokos You don't use them, so either remove them (`int main(void)`) or give them the usual names `argc` and `argv`. Also they are typically not written to. http://stackoverflow.com/questions/9356510/int-main-vs-void-main-in-c –  Jan 26 '14 at 02:37
  • Ok, now when I implemented all the corrections you suggested I get no warnings when compiling anymore. However the numbers returned by the program are different from those in txt file. For example my min in txt is -321.654 and the program shows Minimum = -321.653992, max is 12654986.5684523 and it shows 12654987.000000. Any idea why? Median is still a random number not present in the file at all. – xLokos Jan 26 '14 at 02:45
  • @xLokos Are you talking about the answer of @JonathanLeffler? If so please post your comment under his answer. I do not claim that the error I found is the only one. Your code is hard to read. –  Jan 26 '14 at 02:49
  • 2
    You only get about six sigfigs from a float, so -321.653992 is most likely the closest representable value to -321.654. – tabstop Jan 26 '14 at 02:50
  • Did you remember to initialize `s` and `t` to zero before you start incrementing them? If not, you start with garbage and add good numbers to it, but GIGO — garbage in, garbage out. You could print the values as you go, of course, as a simple way of debugging — what was the value just read, and what are the control variables (`s`, `t`, `a`, `l`, `n`) now set to? – Jonathan Leffler Jan 26 '14 at 02:56
0

This is a cleaned up version of your code…but select Nabla's answer.

It compiles cleanly with:

gcc -O3 -g -std=c11 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \
    -Wold-style-definition -Werror med.c -o med

It shows that you were not dreadfully far off. I have not fixed the initializer for l. I did ensure that s and t are both zeroed before you start accumulating values in them. The code does not check that fscanf() works on the second pass. I've not fixed it to read from standard input or to take the file name from its arguments, either of which would make it a lot more usable.

Given input data:

1.2
3.5
2.9
4.6

it produces the output:

Sample size = 4
Minimum = 1.200000
Maximum = 4.600000
Mean = 3.050000
Median = 3.200000
Standard deviation = 3.288617

Semi-fixed code

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

int compare(const void *a, const void *b);

int compare(const void *a, const void *b)
{
    float fa = *(float *) a;
    float fb = *(float *) b;
    return (fa > fb) - (fa < fb);
}

// median calculations//
float median1(float[], int);

float median1(float array[], int n)
{
    qsort(array, n, sizeof(float), compare);

    if (n % 2 == 0)
        return (array[n / 2] + array[n / 2 - 1]) / 2;
    else
        return array[n / 2];
}

int main(void)
{
    float x;
    float l = ~(257 << 23);
    float a;
    float s = 0.0;
    float t = 0.0;
    float median;
    int n;
    FILE *f;
    char fname[20];
    int i;
    a = -l;

    printf("Please type the file name with an extension (.txt)\n");
    scanf("%s", fname);


    f = fopen(fname, "r");
    if (f == 0)
    {
        fprintf(stderr, "Failed to open file %s for reading\n", fname);
        return 1;
    }

    for (n = 0; fscanf(f, "%f", &x) > 0; n++)
    {
        s += x;
        if (x<l) l = x;
        if (x>a) a = x;
        t += x * x;
    }

    float array[n];

    fseek(f, 0, SEEK_SET);

    for (i = 0; i < n; i++)
    {
        fscanf(f, "%f", &(array[i]));
    }

    fclose(f);

    median = median1(array, n);

    printf("Sample size = %d\n", n);
    printf("Minimum = %f\n", l);
    printf("Maximum = %f\n", a);
    printf("Mean = %f\n", s / n);
    printf("Median = %f\n", median);
    printf("Standard deviation = %f\n", sqrtf(t / n));

    return 0;
}
Community
  • 1
  • 1
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • I can't say it doesn't look much neater. But still even with this code and input `123.456 999.999 564.312 -654.321 7894.32` it produces output `Sample size = 5 Minimum = -654.320984 Maximum = 7894.319824 Mean = 1785.553101 Median = 564.312012 Standard deviation = 3580.003174 ` – xLokos Jan 26 '14 at 02:59
  • 1
    That's what I get too from your data: `Sample size = 5 Minimum = -654.320984 Maximum = 7894.319824 Mean = 1785.553101 Median = 564.312012 Standard deviation = 3580.003174`. It's reasonable given the inputs. Trying to get 10 significant digits from a value that can only hold 6-7 is an exercise in futility, but the default precision for `%f` is 6 decimal places, so that's what gets printed. – Jonathan Leffler Jan 26 '14 at 03:03
  • So do you think this whole code will be accepted by my tutor? – xLokos Jan 26 '14 at 03:08
  • I just checked with my GCC and the stdev is slightly different from this in output. My Casio shows `sample stdev = 3469.18976` – xLokos Jan 26 '14 at 04:28
  • Btw thatnks for your help and time, I appreciate it. – xLokos Jan 26 '14 at 04:30
  • I think the problem with the standard deviation is that you're using ∑x²/n instead of √(∑(nx²-µ²)/(n(n-1))). See [Wikipedia](https://en.wikipedia.org/wiki/Standard_deviation) and [MathsIsFun](http://www.mathsisfun.com/data/standard-deviation-formulas.html) for more information. – Jonathan Leffler Jan 26 '14 at 07:13
  • Why n(n-1)? should it look like t += (x-(s/n))² and return value of stdev = t/n in my case? – xLokos Jan 26 '14 at 12:25
  • Nvm, I've done it another way and it works now. Thank you again ;) – xLokos Jan 26 '14 at 14:12