0

I want to write a struct from queue (data structure) struct using fwrite() into a binary file.

struct addressbook
{
    char first[15];
    char surname[15];
    char address[60];
    char city[15];
    char country[15];
    char zip[6];
    char phone[15];
    char mob[15];
};

struct node
{
    struct addressbook data;
    struct node *next;
};

struct node *front = NULL;
struct node *rear = NULL;

My code is as follows:

void writedata (void)
{
    FILE *fp;

    char CONTINUE;

    struct addressbook blank;
    struct node *temp;

    fp = fopen("asdf.dat","ab");

    while(1)
    {
        temp = (struct node*)malloc(sizeof(struct node));

        if (temp != NULL)
        {
            fflush(stdin);
            printf("\nEnter First name?\n");
            gets(temp->data.first);

            printf("Enter Surname?\n");
            gets(temp->data.surname);

            printf("Enter Street Address (with commas)?\n");
            gets(temp->data.address);

            printf("Enter City?\n");
            gets(temp->data.city);

            printf("Enter Country?\n");
            gets(temp->data.country);

            printf("Enter Zipcode?\n");
            gets(temp->data.zip);

            printf("Enter Phone number?\n");
            gets(temp->data.phone);

            printf("Enter Mobile number?\n");
            gets(temp->data.mob);

            temp->next = NULL;

            fwrite(&temp->data,sizeof(struct addressbook),1,fp);

            if (front == NULL)
            {
                front = temp;
            }

            else
            {
                rear->next = temp;
            }

            rear = temp;

        }

        else
        {
            printf( "No memory available.\n");
        }

        printf("CONTINUE\nY-YES \nN-NO\n? ");
        fflush(stdin);
        CONTINUE = getchar();

        if (CONTINUE == 'N' || CONTINUE == 'n')
            break;
    }

    fclose(fp);
}

I am getting garbage value at the output.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Adoni Aamir
  • 1
  • 3
  • 1
    See [Why the `gets()` function is too dangerous to be used — ever!](http://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used) and then repent of your previous ignorance (revise the code to use `fgets()` or a similar function). Also note the caveats about [Using `fflush(stdin)`](http://stackoverflow.com/questions/2979209/using-fflushstdin). – Jonathan Leffler May 25 '17 at 19:12
  • 1
    You should probably identify what your input is, and the garbage output you get — possibly as a hex dump of the file. You need only show one record. Note that one of the problems with using `gets()` is that you're relying on the user not to enter too much data. You might want to think about reading each value into a big buffer (`char buffer[4096];` for example), and then checking whether what was typed will fit. You should also check each input call to ensure it did not fail. EOF can occur at any time. Please read about how to create an MCVE ([MCVE]), though you're pretty close here. – Jonathan Leffler May 25 '17 at 19:18
  • "I am getting garbage value at the output" - the only output from this code are the prompts to the user for *input*. What output are you referring to, the target file content? Are you expecting something *other* than a block of data, some of which is the input data, the rest being the indeterminate content of the unspecified data in each field? That's all you're going to get. If you have a 15 char indeterminate buffer, populate ten chars with a nine-char string + nullchar, you're going to get exactly what you wrote: 10 chars of determinate data, and five more of indeterminate content. – WhozCraig May 25 '17 at 19:28
  • the posted code does not compile! At the very least, it is missing the `main()` function and the statement: `#include ` – user3629249 May 27 '17 at 02:46
  • While visual studio will allow you to use: `fflush(stdin)` The function: `fflush()` is for output streams, no input streams. Suggest using: `int ch; while( (ch = getchar()) != EOF && '\n' != ch );` – user3629249 May 27 '17 at 02:52
  • the posted code contains several 'magic' numbers. 'magic' numbers are numbers with no basis. I.E. 5, 15, 60. 'magic' numbers make the code much more difficult to understand, debug, etc. Suggest use a `enum` statement or `#define` statements to give those 'magic' numbers meaningful names, then use those meaningful names throughout the code. – user3629249 May 27 '17 at 02:56
  • in function: `writedata()`, the variable `CONTINUE` is the same spelling as the `continue` loop iterator. It is poor program practice to have variable names that are the same as the C operators. Having the same name with the only difference being the capitalization is very confusing to the human reader of the code. Note: it is typical for ALL_CAPS to be used for `#define` and `enum` names. Using variable names that are all caps is confusing to the human reader. – user3629249 May 27 '17 at 03:00
  • when calling `fopen()`, always check (!=NULL) the returned value to assure the operation was successful. If NULL, then call `perror()` then `exit()` The `perror()` will display (on stderr) the text you supplied plus the reason the system thinks the function failed. – user3629249 May 27 '17 at 03:05
  • when calling any of the heap allocation functions: (malloc, calloc, realloc), 1) always check (!=NULL) the returned value to assure the operation was successful. 2) the return value type is `void*` which can be assigned to any other pointer. Casting the returned value just clutters the code, making it more difficult to understand, debug, etc. – user3629249 May 27 '17 at 03:07
  • error messages should be output to `stderr`, not `stdout`, so this line: `printf( "No memory available.\n");` should be: `perror( "malloc failed" );` – user3629249 May 27 '17 at 03:09
  • this line: `fwrite(&temp->data,sizeof(struct addressbook),1,fp); ` should be followed by a check of the returned value from`fwrite()`, AND that would be a good time to pass the pointer returned from `malloc()` to `free()` OR better, reuse the same memory space, then pass to `free()` before exiting the program. – user3629249 May 27 '17 at 03:12
  • you could simplify the statement: `if (CONTINUE == 'N' || CONTINUE == 'n')` by replacing it with: `if( 'N' == toupper( CONTINUE ) )` Note: `toupper()` is available by the statement: `#include ` – user3629249 May 27 '17 at 03:15
  • when the call to `malloc()` fails, it will continue to fail. so when it fails, cleanup (close open files, free allocated memory, etc) then call `exit( EXIT_FAILURE );` where `exit()` and `EXIT_FAILURE` are available by the statement: `#include ` – user3629249 May 27 '17 at 03:19
  • Did you notice that the variable named `blank` is not used, so should be eliminated – user3629249 May 27 '17 at 03:20
  • in the latest version of the C programming language, the function: `gets()` is completely removed. (previously, it was just depreciated). Strongly suggest using `fgets()` to input the values, Note: `fgets()` will include the trailing newline '\n' so that needs to be removed via something similar to: `char *newline = NULL; if( NULL != (newline = strchr( , '\n' ) ) { *newline = '\0'' }` – user3629249 May 27 '17 at 03:26
  • just to give you an example, this line: `gets(temp->data.first); should be: `if( !fgets( temp->data.first, sizeof( struct addressbook.first), stdin ) ) { perror( "fgets for first name failed" ); .... }` Note: if meaningful names are used for the size of each field then replace: `sizeof( struct addressbook.first)` with that meaningful name – user3629249 May 27 '17 at 03:33
  • to avoid having garbage in the data written to the file, after calling `malloc()` and before setting any fields, insert the statement: `memset( temp, '\0', sizeof( struct node) )` – user3629249 May 27 '17 at 03:37
  • to avoid confusing the user, in the prompts for `Phone number` and `Mobile number`, include the expected format in the prompt. something similar to `(###) ###-####` And remember that the fields are only 15 bytes wide and so the actual number must be less than13 characters (remember the elimination of the trailing newline and `fgets()` automatically appends a '\0' byte – user3629249 May 27 '17 at 03:42
  • Note: in America the `zipcode` is a combination of a 5byte number and a 4 byte number. Which means that the `zip` field is way too short – user3629249 May 27 '17 at 03:45

0 Answers0