[Sapperm]

I am working on some homework and my task is to create a linked list of items and fill that list from a file that has a set format. Below is the two functions that I am working on. The first function takes in the file and opens it to start reading lines. As of now, the code reads in the last line(item) and prints it out via the second function below. My goal
is to print out all items from the file no matter the number of lines. I created the head of the list and malloced it
and can populate only one item. I need to find the way to traverse through a linked list adding nodes as it reads in data from the file.

My thoughts of how to continue involve creating the linked list and utilizing my while loop for the fscanf() to populate each node.

item_t *ReadItemsFromFile(char *file)
{
 printf("entered the function!\n");
   item_t *base;
   item_t *pointer;
   base = malloc(sizeof(item_t));
   base->next = NULL;
   pointer = base;


   char *fpin;

fpin = fopen(file, "r");


while(fscanf(fpin, " '%[^']'  %d %f %f",base->name,&base->dam,&base->cost,&base->weight) == 4)
      printf("Read the line!\n");
 return base;

void PrintItemTable
(
   FILE *fpout,    /* FILE stream to print to (set to stdout for screen) */
   item_t *itb     /* list of items to dump to screen */
)
{
    int num = 0;

   /* declarations here! */
   fprintf(fpout,
   " NUM  NAME                             DAMAGE          COST    WEIGHT\n");
   fprintf(fpout,
   "====  ===============================  ======  ============  ========\n");
   fprintf(fpout,
   "%4d %31s  %7d %13.2f %9.2f   \n",num,itb->name ,itb->dam, itb->cost, itb->weight );

 /* print all the lines */
    printf("\n");
}

[baavgai]

I'd start like:

item_t *ReadItemsFromFile(char *file) {
	printf("entered the function!\n");
	item_t *base = NULL; // you gotta start somewhere
	// don't do this now, you just started 
	// base = malloc(sizeof(item_t));

	// no, this should be file
	// char *fpin;
	FILE *fpin = fopen(file, "r");
	if (fpin) { // check to see if you actually have an open file
		item_t item; // to store your results
		while(fscanf(fpin, " '%[^']'  %d %f %f", item.name, item.dam, item.cost, item.weight) == 4) {
			// time to build the list
			item_t *node = malloc(sizeof(item_t));
			*node = item; // copy item to node
			node->next = //....

[#define]

The next pointer at the end of the list should always be NULL, so you can loop through the list until next is NULL.

[Sapperm]

Thanks for the help. I will look and study all of this when I get home from work. I see in the while loop you changed the assignments from -> to . Is that b/c of the format from the initial pointers changed? Also under the while loop when we are creating more nodes, are they filling in like the original one with it continuing until it reaches EOF? So if so I would be able to return the base pointer and use another function to traverse the list ( as in the print table function)

Again thanks for the help. Glad to always learn.

[baavgai]

Sapperm, on 27 November 2012 - 02:19 PM, said:

I see in the while loop you changed the assignments from -> to . Is that b/c of the format from the initial pointers changed?

No. That instance of your struct is concrete, not a pointer. There is no sense if allocating if you don't have to. In this case, you need a bucket to dump your read. Only after you read do you decide to keep it or not.

You could go with creating a base node up front, like you're doing, but then you need be concerned about deleting it the last unused node. Dynamic memory allocation is good, avoiding unnecessary allocation and deallocation it is better.

Sapperm, on 27 November 2012 - 02:19 PM, said:

Also under the while loop when we are creating more nodes, are they filling in like the original one with it continuing until it reaches EOF?

If you create a stack style implementation, the simplest, then you only need point your next to the last head, reassign head, and keep on going. Don't loose your head.

[Sapperm]

Quote

No. That instance of your struct is concrete, not a pointer. There is no sense if allocating if you don't have to. In this case, you need a bucket to dump your read. Only after you read do you decide to keep it or not.

thanks that really made it click for me

[Sapperm]

Just an update (long overdue), but I got the code working perfectly thanks to the fine people on these boards. Finished code for the problem section below.

item_t *ReadItemsFromFile(char *file)
{
 printf("entered the function!\n");
   item_t *base;
   item_t *pointer;
   base = malloc(sizeof(item_t));
   base->next = NULL;
   pointer = base;


   FILE *fpin;

fpin = fopen(file, "r");


while(fscanf(fpin, " '%[^']'  %d %f %f",pointer->name,&pointer->dam,&pointer->cost,&pointer->weight) == 4)
{
      printf("Read the line!\n");

    pointer->next = malloc(sizeof(item_t));
    pointer = pointer->next;
}
pointer->next = NULL;
return base;

[baavgai]

Glad you got it working.

You still have a spare node on the tail:

item_t *ReadItemsFromFile(char *file) {
	printf("entered the function!\n");
	item_t *base;
	item_t *pointer;
	
	base = malloc(sizeof(item_t)); /* allocate an instance of item_t */
	base->next = NULL;
	pointer = base;


	FILE *fpin = fopen(file, "r");
	/* try to read into current empty node */
	while(fscanf(fpin, " '%[^']'  %d %f %f",pointer->name,&pointer->dam,&pointer->cost,&pointer->weight) == 4) {
		printf("Read the line!\n");
		/* if the scanf succeeds, a node and create another dummy node */
		pointer->next = malloc(sizeof(item_t));
		pointer = pointer->next;
	}
	pointer->next = NULL; /* cap the current empty node, which must be ignored */
	return base;
}

You could avoid this like so:

item_t *ReadItemsFromFile(char *file) {
	FILE *fpin;
	item_t *head = NULL;
	
	printf("entered the function!\n");
	
	/* we should really cuck to see if we opened the file */
	if ((fpin = fopen(file, "r")) == NULL) {
		printf("error opening file: %s\n", file);
	} else {
		item_t data, *tail;
		while(fscanf(fpin, " '%[^']'  %d %f %f", data.name, &data.dam, &data.cost, &data.weight) == 4) {
			item_t *node = malloc(sizeof(item_t));
			*node = data; /* load the data */
			node->next = NULL;
			if(head==NULL) {
				head = tail = node;
			} else {
				tail->next = node;
				tail = tail->next;
			}
		}
		/* do NOT forget to close that file you just opened */
		fclose(fpin);
	}
	return head;
}

[Sapperm]

Good catch, I deleted the node as such.
I understand that I used a lesser efficient way, but I guess it is that way that makes more sense to mt at the moment. Thanks again for all of the pointers. (no pun intended)

item_t *ReadItemsFromFile(char *file)
{
 printf("entered the function!\n");
   item_t *base;
   item_t *pointer;
   base = malloc(sizeof(item_t));
   base->next = NULL;
   pointer = base;


   FILE *fpin;

fpin = fopen(file, "r");


while(fscanf(fpin, " '%[^']'  %d %f %f",pointer->name,&pointer->dam,&pointer->cost,&pointer->weight) == 4)
{
      printf("Read the line!\n");

    pointer->next = malloc(sizeof(item_t));
    pointer = pointer->next;
    pointer->next = NULL;
}
   item_t *tempread;               /*used to delete extra node */
   tempread = base;
   item_t *tempold;
   while(tempread->next!=NULL)
   {
      tempold = tempread;
      tempread = tempread->next;

   }
   tempold->next = NULL;
   free(tempread);


   fclose(fpin);

return base;

[baavgai]

Excellent. I'm glad you're playing with it.

You needn't go through the second looping. Really, you only need the previous node so you can set it's next to null, chopping off the tail. Your pointer variable is the node you want to free, so that's easy. Keep in mind, the less variables in your code block the less you have to worry about.

e.g.

item_t *ReadItemsFromFile(char *file) {
	printf("entered the function!\n");
	item_t *base, *pointer, *prev_pointer;

	base = malloc(sizeof(item_t));
	base->next = NULL;
	pointer = base;
	prev_pointer = NULL;

	FILE *fpin = fopen(file, "r");

	while(fscanf(fpin, " '%[^']'  %d %f %f",pointer->name,&pointer->dam,&pointer->cost,&pointer->weight) == 4) {
		printf("Read the line!\n");
		prev_pointer = pointer;
		pointer->next = malloc(sizeof(item_t));
		pointer = pointer->next;
		pointer->next = NULL;
	}
	if (prev_pointer!=NULL) { prev_pointer->next = NULL; }
	free(pointer);

	fclose(fpin);
	return base;
}

You still don't check to see if you actually opened a file. You really should. If you don't, you'll crash somehow. Always better to give back some idea of why something failed than to keep it a secret.