0

I'm trying to display my output in a 4X4 format. The program receives name of a text file then get the values in the text file and store in the 2D array. Then later displays them like this

7 2 4 5 
5 2 6 4
2 2 5 5 
9 2 4 5

But the problem is, it doesn't display like that, I'm not sure if it's my loop or what. Any ideas. it runs fine with no errors but the numbers don't display right. Here's my code

int main () {

  int i = 0;
  int j = 0;

   int value = 0;
   int a[4][4];
   char ch, file_name[100];
   FILE *fp;

   printf("Enter file name?");
   gets(file_name);

   fp = fopen("file.txt", "r");    

   if (fp == NULL)
   {
      perror("Error \n");
   }
   else 
   {

       // printf("\n");
        while (!feof(fp) && fscanf (fp, "%d ", &value))
        {
            a[i][j] = value;

         for ( i = 0; i < 4; i++ ) 
                {
              for ( j = 0; j < 4; j++ ) 
              {
                 printf("%d ",a[i][j] );
              }   
             printf("\n");      

           }     
       }
   }

    fclose(fp);

   return 0;

}
Lundin
  • 195,001
  • 40
  • 254
  • 396
ShadowWalker
  • 13
  • 2
  • 7
  • 1
    Please see [Why is `while ( !feof (file) )` always wrong?](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) You should control the loop with the return value from `fscanf`. Such as `while (fscanf (fp, "%d ", &value) == 1) { }` – Weather Vane Mar 16 '18 at 22:17
  • 1
    Ack! `gets` is never to be used! – William Pursell Mar 16 '18 at 22:17
  • https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used#1694042 – William Pursell Mar 16 '18 at 22:18
  • @WeatherVane I changed that it still didn't change anything. – ShadowWalker Mar 16 '18 at 22:23
  • Oh an aside: please also remove the trailing space from `"%d "`. – Weather Vane Mar 16 '18 at 22:26
  • @WeatherVane: You need the trailing space to separate multiple numbers on a line. You just don't need it on the last one (though it's mostly harmless). If you want to get fancy (not tested): `printf("%d%c", a[i][j], j == 3 ? '\n' : ' ');` – Keith Thompson Mar 16 '18 at 22:33
  • The code you've shown us is not your entire program. You're missing one or more `#include` directives. Don't assume that we know they're there. See [mcve]. – Keith Thompson Mar 16 '18 at 22:35
  • @KeithThompson there is no `%c` in the code. The `%d` filters leading whitespace without any intervention. – Weather Vane Mar 16 '18 at 22:35
  • @WeatherVane: Oh, you were talking about the `scanf` call. I was referring to the `printf` call a few lines down. – Keith Thompson Mar 16 '18 at 22:38
  • 2
    "the numbers don't display right" is not an acceptable problem statement. – Lightness Races in Orbit Mar 16 '18 at 22:43
  • Tried all these and I can't get the right format still. I'm thinking the issue is coming from the for loop. When I remove the for loop then it displays the numbers in vertically or horizontally but with the for loop it just displays a lot of different numbers plus negative numbers – ShadowWalker Mar 16 '18 at 22:46

2 Answers2

1

Your code suffers from various problems since you are mixing the code that reads the input and code that writes the output in one while loop.

Separate the code that reads the input and the code that creates the output. Ideally, put them into different functions.

Declare the functions as:

void readData(FILE* in, int a[4][4]);
void writeData(FILE* out, int a[4][4]);

Use them as:

int main()
{
   int a[4][4];

   // Your code to open the file.

   readData(in, a);

   writeData(stdout, a);
}

Implement them as:

void readData(FILE* in, int a[4][4])
{
   for ( i = 0; i < 4; i++ ) 
   {
      for ( j = 0; j < 4; j++ ) 
      {
         if ( fscanf(in, "%d", &a[i][j]) != 1 )
         {
            printf("Unable to read an array element.\n");
            exit(0);
         }   
      }   
   }
}

void writeData(FILE* out, int a[4][4])
{
   for ( i = 0; i < 4; i++ ) 
   {
      for ( j = 0; j < 4; j++ ) 
      {
         fprintf(out, "%d ", a[i][j]);
      }   
      fprintf(out, "\n");      
   }
}
R Sahu
  • 204,454
  • 14
  • 159
  • 270
0

Well, let's take your problems apart a piece at a time. First, the salient part of your code regarding input and attempted output:

    int i = 0;
    int j = 0;
    int value = 0;
    int a[4][4];
    ...
    while (!feof(fp) && fscanf (fp, "%d ", &value))
    {
        a[i][j] = value;

     for ( i = 0; i < 4; i++ ) 
            {
          for ( j = 0; j < 4; j++ ) 
          {
             printf("%d ",a[i][j] );
          }   
         printf("\n");      

       }     
   }

Before we look at the !feof problem. let's look at the overall logic of your loop structure. When you enter your while loop, the values of i = j = 0;. Presuming your file is open and there is an integer to read, you will fill value with the first integer in the file and then assign that value to the first element of your array with:

a[i][j] = value;

Now, you have stored 1-integer at a[0][0]. (all other values in a are uninitialized and indeterminate) Inexplicably, you then attempt to output the entire array, ... uninitialized values and all.

     for ( i = 0; i < 4; i++ ) 
            {
          for ( j = 0; j < 4; j++ ) 
          {
             printf("%d ",a[i][j] );
          }   
         printf("\n");      

       }     

Attempting to access an uninitialized value invokes Undefined Behavior (your program is completely unreliable from the first attempt to read a[0][1] on). It could appear to run normally or it could SegFault or anything in between.

You need to complete the read of all values into your array before you begin iterating over the indexes in your array outputting the values.

But wait... there is more... If you did not SegFault, when you complete the for loops, what are the values of i & j now? Your loops don't complete until both i and j are 4, so those are the values they have at the end of the first iteration of your while loop.

Now let's go to the next iteration of your while loop. Presuming there are two integer values in the file you are reading, you read the second integer into value. What happens next? You one-up yourself. After already invoking undefined behavior 15 times by attempting to read 15 uninitialized values a[0][1] -> a[3][3], you then attempt to write beyond the bounds of a with:

a[4][4] = value;         /* remember what i & j are now? */

You then hit your for loops..., again with a[0][1] holding the only validly initialized value and the cycle repeats.

But wait..., there is more... After having read the last integer in your file, you arrive at the beginning of your while loop one last time:

    while (!feof(fp) && fscanf (fp, "%d ", &value))

You test !feof(fp) and no EOF has been set because your last read was a valid read of the last integer which completed with the last digit and did not trigger a stream error, so you proceed to fscanf (fp, "%d ", &value) which now returns EOF (e.g. -1), but since you simply test whether fscanf(..), -1 tests TRUE so off you go again through the entire body of your loop invoking undefined behavior at least 16 more times.

Are you beginning to understand why the output may not have been what you wanted?

You have already had a good answer on how to go about the read and print. I'll offer just an alternative that does not use any functions so it may help you follow the logic a bit better. The following code just reads from the filename given as the first argument, or from stdin by default if no argument is given. The code validates that the file pointer points to a file that is open for reading and then enters a while loop reading an integer at a time into &array[rows][cols] with fscanf and validating the return of fscanf is 1. Anything else is considered failure.

The remainder of the read loop just increments cols until is reaches NCOLS (4 here - being the number of columns per-row), then it increments the row-count in rows and sets the column-index cols back to zero.

A final validation is performed before output to insure NROWS of integers were read and that cols was set to zero during the final iteration of the read-loop. The contents of array is then output. The code is fairly straight-forward:

#include <stdio.h>

#define NROWS 4
#define NCOLS NROWS

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

    int array[NROWS][NCOLS] = {{0}},    /* declare and intialize array */
        rows = 0, cols = 0;             /* rows / columns index */
    FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;

    if (!fp) {  /* validate file open for reading */
        fprintf (stderr, "error: file open failed '%s'.\n", argv[1]);
        return 1;
    }

    /* validate no more than NROWS of NCOLS integers are read */
    while (rows < NROWS && fscanf (fp, "%d", &array[rows][cols]) == 1) {
        cols++;                 /* increment column */
        if (cols == NCOLS) {    /* if row full */
            rows++;             /* increment rows */
            cols = 0;           /* zero columns */
        }
    }
    if (fp != stdin) fclose (fp);   /* close file if not stdin */

    if (rows != NROWS || cols) {    /* validate rows = NROWS & cols = 0 */
        fprintf (stderr, "error: read only %d rows and %d cols.\n", 
                 rows, cols);
        return 1;
    }

    for (int i = 0; i < NROWS; i++) {   /* output stored values */
        for (int j = 0; j < NCOLS; j++)
            printf (" %2d", array[i][j]);
        putchar ('\n');
    }

    return 0;
}

(you have already been advised not to use gets which is subject to easy buffer overflow and so insecure it has been completely removed from the C11 library. If your professor continues to suggest its use -- go find a new professor)

Example Input File

$ cat dat/arr2dfscanf.txt
7 2 4 5
5 2 6 4
2 2 5 5
9 2 4 5

Example Use/Output

$ ./bin/arr2dfscanf <dat/arr2dfscanf.txt
  7  2  4  5
  5  2  6  4
  2  2  5  5
  9  2  4  5

Also note, that by reading with fscanf in this manner, it doesn't matter what format your input is in -- as long as it is all integer values. This will produce the same output:

$ echo "7 2 4 5 5 2 6 4 2 2 5 5 9 2 4 5" | ./bin/arr2dfscanf

Look things over and let me know if you have further questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • Thanks @David C Rankin. I understand the Logic and I like how you explained it. When I run the code, it just compiles but doesn't output anything. – ShadowWalker Mar 17 '18 at 00:09
  • Did you give it an input file? Or send it integers on `stdin` like it did above with the `"echo"` statement? – David C. Rankin Mar 17 '18 at 00:17
  • Yes. So something like this? FILE *fp = argc > 1 ? fopen (argv[('a.txt')], "r") : stdin; – ShadowWalker Mar 17 '18 at 00:19
  • NO. Just put `a.txt` on the command line as the first argument. e.g. `./program a.txt` Or *redirect* `a.txt` to the program on `stdin`, e.g. `./program – David C. Rankin Mar 17 '18 at 00:19
  • The line `FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;` just says assign the file pointer to: (1) if `argc > 1` (meaning there is an argument), then `fopen (argv[1], "r")` and read from the filename given as `argv[1]` -or- (2) if no argument is given, read from `stdin`. It is just the use of the *ternary* operator to read using `argv[1]` if given, else use `stdin` for input (it's a shortcut, sorry it was confusing) Let me know if you have any more problems. I'm happy to help until you get it sorted out in your mind. – David C. Rankin Mar 17 '18 at 00:24
  • So i will just have to use stdin to get the text file? I don't want to use the command line. I just want to be able to run the program from the ide and get the text file. – ShadowWalker Mar 17 '18 at 00:27
  • Let's do this. change `FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;` to `FILE *fp = fopen ("a.txt", "r");` and we will forget about the file or `stdin` trick. This presumes your numbers are in `a.txt` of course. If not, just give the filename instead of `a.txt` that holds your numbers. – David C. Rankin Mar 17 '18 at 00:29
  • Worked like a charm. You the best.Thank you – ShadowWalker Mar 17 '18 at 00:33
  • Good glad it helped. Good luck with your coding. – David C. Rankin Mar 17 '18 at 02:40