0

I have on my local machine a file named "data.in" with this content:

1
5
6
6
8
10
33
24
20
3

And the source code:

#include <stdio.h>

int main (void)
{
    int n,i,a,V[i],ch,aux;
    FILE *f1, *f2;

    f1 = fopen("data.in", "r");
    f2 = fopen("data.out", "w"); //create data.out

    char line[1024];
    n = 0;
    while( fgets(line,sizeof(line),f1) != NULL)
       n++; // n = number of lines from the file

    for (i=0; i<n; i++)
        fscanf(f1,"%d", &V[i]); //reading the array from data.in

    do {
        ch=0;
        for (i=0; i<n-1; i++)
            if (V[i]>V[i+1])
            {
                aux=V[i]; V[i]=V[i+1]; V[i+1]=aux; ch=1;
            }
    } while (ch); //Bubble sort

    for (i=0; i<n; i++)
        fprintf(f2, "%d\n", V[i]); // print the array into data.out

    fclose(f1);
    fclose(f2);

}

The compilation goes fine but whenever I execute it, data.out contains only:

0
0
0
0
0
0
0
0
0
0

I even tried to print only the array but it is still a bunch of zeros. I even tried to modify data.in to have all the numbers on the same line but the output was still only a bunch of zeros. I must be missing something...

I'm kind of stuck here so Any help would be appreciated.

7
  • 3
    Please indent your code blocks, otherwise it is very hard to read and to understand it. Commented Jul 21, 2013 at 9:23
  • 2
    I guess your program runs at the end of the file in the lines counting cycle and than you just read 0. Try to reopen the file or seek at the begining of the file to read it again. Commented Jul 21, 2013 at 9:24
  • 3
    int n,i,a,V[i], I'm not quite sure what you expect from that declaration of V. And whatever it is you expect, I'm pretty sure you're not getting it. Commented Jul 21, 2013 at 9:25
  • 3
    I'm almost sure V[i] is wrong. I don't know how is your code running Commented Jul 21, 2013 at 9:27
  • 2
    No; the reading code is incorrect. You read the whole file to find out how big it is, then you continue reading from the end. You must rewind before rereading. If you bothered to check the return statuses from the functions you called, they would tell you that there were problems (the fscanf() calls would return EOF, for example). You should check that your fopen() calls succeed too, though they must be OK in fact since your code is not crashing. You should also defer the definition of V until you know the value of n and you can write int V[n];. Commented Jul 21, 2013 at 9:37

5 Answers 5

2

If you use dynamic memory allocation, you neither need to define the array of a fixed size and run the risk of needing more space than you've allocated nor need to reread the file. (On the other hand, for files of a few dozen — or even a few thousand — numbers, this is probably overkill.)

You could also use the standard library sort function, qsort(), rather than use a bubble sort. Granted, for the size of data you're dealing with, the difference between qsort() and bubble sort is not likely to be easily measured, but if you move from tens of numbers to thousands of numbers, the difference between an O(N2) and an O(N log N) algorithm becomes apparent. (See How to sort an array of structures in C? for a discussion of why intcmp() below is written as it is.)

Also, you should error check input operations (and memory allocation). Using a simple function like the err_exit() function shown in the code makes error reporting succinct, and therefore less onerous and reduces the excuses for omitting the error checking. I use a more featureful variant of err_exit() in the majority of my programs, but that is code in its own source file with its own header. Many programs (including the rewrite below) do not check output operations for success; they probably should.

This leads to code similar to this:

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

static int intcmp(const void *p1, const void *p2);
static void err_exit(const char *fmt, ...);

int main(void)
{
    static const char n1[] = "data.in";
    static const char n2[] = "data.out";
    FILE *f1 = fopen(n1, "r");
    FILE *f2 = fopen(n2, "w");
    int *V = 0;
    char line[1024];
    int n = 0;
    int max_n = 0;

    if (f1 == 0)
        err_exit("Failed to open file %s for reading\n", n1);
    if (f2 == 0)
        err_exit("Failed to open file %s for writing\n", n2);

    while (fgets(line, sizeof(line), f1) != NULL)
    {
        int v;
        if (sscanf(line, "%d", &v) != 1)
            break;
        if (n == max_n)
        {
            int new_n = (max_n + 2) * 2;
            int *new_V = realloc(V, new_n * sizeof(*V));
            if (new_V == 0)
                err_exit("Failed to realloc array of size %d\n", new_n);
            V = new_V;
            max_n = new_n;
        }
        V[n++] = v;
    }

    qsort(V, n, sizeof(V[0]), intcmp);

    for (int i = 0; i < n; i++)
        fprintf(f2, "%d\n", V[i]);

    free(V);
    fclose(f1);
    fclose(f2);
    return(0);
}

static int intcmp(const void *p1, const void *p2)
{
    int i1 = *(int *)p1;
    int i2 = *(int *)p2;
    if (i1 < i2)
        return -1;
    else if (i1 > i2)
        return +1;
    else
        return 0;
}

static void err_exit(const char *fmt, ...)
{
    va_list args;
    va_start(args, fmt);
    vfprintf(stderr, fmt, args);
    va_end(args);
    exit(1);
}
Sign up to request clarification or add additional context in comments.

4 Comments

Thank you sir! This type of code is surely top-quality, but is very hard for me to fully understand, at this point. I wanted something simple without to many functions. I copied your code and when I will get to fully understand everything stated inside, I will surely enjoy re-reading it!
Yes; my answer is overkill for your immediate problem. The rewind operation and fixing the definition of V are sufficient. You should add error checking to your code; having your program crash because data.in is missing or data.out cannot be created/truncated is not good. If you go with a fixed-size array, you should ensure you don't overflow it. If you go with a VLA, you're at the mercy of your runtime; if the size of the array is too big, your code will crash.
Using and writing functions is important. The err_exit() function is 8 lines as written (plus 1 for the function definition). Each 1-line invocation of it replaces 4 lines of code (open brace, fprintf(), exit(), close brace), so each use saves three lines of code. In the code shown with three uses, the saving is nil, but the the code in main() is simpler (better) because of it. Use functions for common jobs. Granted, it uses techniques you've probably not been taught yet, but it is actually very simple and the function is basically boilerplate. I've typed that code many times.
+1 Your answer is very comprehensive and err_exit is quite impressive to me. I learned much.
1

After you count the number of lines in the file, the fi has been changed.

You need to reset fi like:

fseek(f1, 0, SEEK_SET);

And read the file from the beginning again.

Then you may get the right output in "data.out".

4 Comments

That is one necessary change. You still have a problem with the definition of the array V.
@JonathanLeffler Yes, I suppose that he/she knows the problem about V. Without knowing this, the code cannot even be successfully compiled.
Actually, with a C99 compiler, gcc -std=c99 -Wall -Wextra -O -c x39.c, the original code gives the output: x39.c: In function ‘main’: and x39.c:5: warning: unused variable ‘a’. It does compile, even under quite stringent warning levels. It just doesn't work well.
@JonathanLeffler Sorry, I didn't see the definition of i which is before definition of V. So I thought it does not compile in the beginning. During my test, I hard-coded V[i] to V[100]. You're right, it does compile.
1

Aside from your declaration of V (what is the value of i? Hint: it may be zero, it may be -2147483648), you used fgets to get the number of lines earlier until the end of the file. You need to rewind(f1); after that, so you can read the file again. Otherwise you end up reading nothing with fscanf.

Might I suggest using fgets and while still in the loop for fgets, also use sscanf to get the string from the line you read? Why read the entire file twice?

while (fgets(line, sizeof line, f1) != NULL) {
    sscanf(line, "%d", &V[n]);
    n++;
}

You should do error checking on the return value of sscanf, but the general idea is in the code. Then you don't need that for loop, nor do you need to rewind the file to read it twice.

1 Comment

You are right, the code will be much more efficient like this!
1

For quick fix, you have to change your array declaration V[i] from int ...,V[i]..., into V[2000]; It's because when allocating an array, you have to know how much items will it have, for example for V[2000] it will have 2000 items with indexes 0 to 1999.

In C99 you can use a variable to get different array size runtime... but you have to have a defined value, i is not clearly known at the line.

Then, you don't know how many lines you have in your file, simplest thing is to just fix the array size and make som control to be sure you wont oveflow your array.

Change your code like this:

const int my_max_numbers = 10; // test it with more than 10 items and change for your likings
int n,i,a,ch,aux;
int V[my_max_numbers];
...

If you want to declare an array of right size you could change your original code differently, do not declare array V at the beginning of file, but after you read the lines and counted the number of lines, you have to use C99 standard.

while( fgets(line,sizeof(line),f1) != NULL)
    n++; // n = number of lines from the file

int V[n];
// here you have to rewind the file to the beginning
fseek(f1,0L,SEEK_SET);

for (i=0; i<n; i++)
    fscanf(f1,"%d", &V[i]); //reading the array from data.in

Manpage for fseek

1 Comment

With your declaration and "fseek(f1, 0, SEEK_SET);" after counting it works correctly now, thanks!
0

I think your V[i] array should be initialized before it was used

3 Comments

As written, V is a VLA (variable-length array) and you cannot write an initializer for a VLA. And, if the next thing you do is read values into the array, there isn't much need to initialize it; the reading operation effectively initializes it.
should he use malloc?
The program could be written to use malloc(), but you wouldn't be able to write an initializer for the dynamically allocated array either. You can only write initializers for fixed size arrays.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.