Array based Linked List in C, issues reading in all data

Question

I am trying to use an array-based linked list to read in values from data.txt and then display those items. The list does not need to be ordered. Currently, it is only printing the last item on the list. I am not positive if I am reading the entire list currently or just the last word. I would appreciate any advice. I have made some changes since earlier. It compiles but I get a segfault which valgrind states "Invalid write or size 1".

main.c

#include <stdio.h>

#include <stdlib.h>
#include <string.h>

#define WORD_SIZE 25
#define MAX_NODES 100
#define FILENAME "data.txt"

char buffer[WORD_SIZE];
int count = 0, i = 0;

typedef struct WORDNODE {
    char word[WORD_SIZE];
    struct WORDNODE *next;
} t_word_node;

t_word_node node_array[MAX_NODES];
t_word_node *free_list = NULL;
t_word_node *tail = NULL;

t_word_node *create_node(void);
void add_word(char *buffer, t_word_node **);

void node_init() {
    for(int count = 0; count < MAX_NODES-2; count++) {
        node_array[count].next = &node_array[count+1];
    }
    node_array[MAX_NODES - 1].next = NULL;
    free_list = &node_array[0];
}

void add_word(char *buffer, t_word_node **list) {
    t_word_node *temp = NULL;
        t_word_node *last = NULL;
        t_word_node *this = *list;

    temp = create_node();
    strcpy(temp->word, buffer);
    while(this && strcmp(buffer, this->word) > 0) {
        last = this;
        this = this->next;
    }
    if(last == NULL) {
            temp->next = this;
                *list = temp;
        } else {
            last->next = temp;
        temp->next = this;
    }
}


t_word_node *create_node() {
    t_word_node *new = NULL;

    if(free_list != NULL)
    {
        new = free_list;
        free_list = free_list->next;
    }
    return new;
}

void print_nodes(t_word_node *this) {
        if(this == NULL) {
                return;
        }
        while(this != NULL) {
                printf("%s\n", this->word);
                this = this->next;
                }
}

int main() {
    count = 0;
    FILE *infile;
    t_word_node *new_list = NULL;
    node_init();
    if((infile = fopen(FILENAME, "r")) != NULL) {
        while (fscanf(infile, "%s", buffer) != EOF) {
                    add_word(buffer, &new_list);
                }
            fclose(infile);
        }
    print_nodes(new_list);
        return 0;
}

data.txt

Kaleidoscope
Rainbow
Navy
Highway
Printer
Junk
Milk
Spectrum
Grapes
Earth
Horse
Sunglasses
Coffee-shop
Ice
Spice
Tennis racquet
Drill
Electricity
Fan
Meat
Clown
Weapon
Drink
Record
Spiral
Air
Data Base
Cycle
Snail
Child
Eraser
Meteor
Woman
Necklace
Festival
Eyes
Foot
Diamond
Chocolates
Explosive
Web
Rifle
Leather jacket
Aeroplane
Chisel
Hose
Flower
Space Shuttle
Radar
Hieroglyph
Ice-cream
Insect
Feather
Ears
Square
Software
Cup
Comet
Church
PaintBrush
Slave
Elephant
Sphere
Aircraft Carrier
Fork
Needle
Prison
Solid
Window
Dress
Knife
Spot Light
Onion
Horoscope
Chief
Crystal
Coffee
Microscope
Freeway
Fruit
Kitchen
Desk
Spoon
Pyramid
Restaurant
Adult
Potato
Egg
Telescope
Family
Sports-car
Television
Jet fighter
Thermometer
Wheelchair
X-ray
Sun
Worm
Church

..or, at least print some stuff out! How about 'buffer', just before you strcpy it into the node? — Martin James
– Martin James, Commented Dec 7, 2020 at 23:53
How about maybe initializing head to NULL so you don't have undefined behavior. And while you're at it, question why you need to search the entire list from the head just to find the tail every time around the loop. Perhaps this was a "solution" to the fact that you're also not initializing tail. — paddy
– paddy, Commented Dec 7, 2020 at 23:54
@paddy as file-scope vars, are not head, tail and temp not initialized to 0 anyway? — Martin James
– Martin James, Commented Dec 7, 2020 at 23:56
@MartinJames sorry not great with the debugger yet, thanks for the print idea! Was able to verify I am reading everything in but I wasn't updating things correctly. — devronn008
– devronn008, Commented Dec 7, 2020 at 23:58

David C. Rankin · Accepted Answer · 2020-12-08 00:04:07Z

1

The problem is you are misusing your tail pointer as a temp pointer. Use temp for a temporary pointer. Next validate that every word read will fit in an array of WORD_SIZE and validate EVERY allocation.

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

#define WORD_SIZE 25
#define FILENAME "data.txt"

typedef struct WORDNODE {
    char word[WORD_SIZE];
    struct WORDNODE *next;
} t_word_node;

t_word_node *head, *tail, *temp;

t_word_node *create_node()
{
    return(t_word_node *)malloc(sizeof(t_word_node));
}

int main (int argc, char **argv) {
    
    char buffer[WORD_SIZE];
    FILE *infile;

    infile = fopen(argc > 1 ? argv[1] : FILENAME, "r");

    while (fgets (buffer, sizeof(buffer), infile) != NULL) {
        size_t len;
        buffer[(len = strcspn (buffer, "\n"))] = 0;
        if (len >= WORD_SIZE) {
            fputs ("error: word exceeds WORD_SIZE\n", stderr);
            continue;
        }
        if (!(temp = create_node())) {
            perror ("malloc-create_node()");
            return 1;
        }
        strcpy(temp->word, buffer);
        temp->next = NULL;
        if (head == NULL)
            head = tail = temp;
        else {
            tail->next = temp;
            tail = temp;
        }
    }
    temp = head;
    while (temp != NULL) {
        puts (temp->word);
        temp = temp->next;
    }
    return 0;
}

You have a tail pointer -- there is never a need to iterate over the list to insert-at-end (that's what the tail pointer is for). Note above how tail is initialized when the first node is added to the list.

Example Use/Output

Do not hardcode filenames. That's what argc and argv are for in int main (int argc, char **argv), pass the filename to read as the first argument to your program or read from FILENAME by default if no argument is provided:

$ ./bin/llarraywords dat/datallwords.txt
Kaleidoscope
Rainbow
Navy
Highway
Printer
Junk
Milk
Spectrum
Grapes
Earth
Horse
Sunglasses
...
<snip>
...
Egg
Telescope
Family
Sports-car
Television
Jet fighter
Thermometer
Wheelchair
X-ray
Sun
Worm
Church

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

edited Dec 8, 2020 at 0:04

answered Dec 7, 2020 at 23:54

David C. Rankin

85.1k6 gold badges67 silver badges95 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

devronn008 Over a year ago

Thanks! the not hardcoding the filename isn't something I've gone over before.

David C. Rankin Over a year ago

It goes to this. You should never have to recompile your program just to read from another file :) Oops I missed one other lesson, never, ever, ever put spaces around the -> (arrow operator) when accessing struct members through a pointer.. (really really bad form) Fixing.

devronn008 Over a year ago

Thanks, that makes sense, is the no spaces thing just a best practice or is there a practical reason?

David C. Rankin Over a year ago

It's a hanging offense in some circles.... :) The compiler doesn't care, it ignores all whitespace not within strings during compilation. This is more a practical style convention -- but just like using ALLCAPS variable names, it speaks volumes about the programmer. (by the way ALLCAPS are reserved for macro and constant names, while all lowercase is used for variable and function names). MixedCase and camelCase are discouraged and are style conventions for C++.

David C. Rankin Over a year ago

When learning linked-lists, there is really only one way to do it. pull out an 8.5 x 11 sheet of paper and pencil. Take the first 3 nodes in your list. When you create_node() create a box on the paper for your 1st node. Follow through the rest of your read-loop and annotate where the ->next pointer points (what address it holds as its value). Do that for the next few iterations until you understand how the nodes are joined.

|

Collectives™ on Stack Overflow

Array based Linked List in C, issues reading in all data

1 Answer 1

7 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

7 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related