Skip to main content
edited body
Source Link
toolic
  • 15.8k
  • 6
  • 29
  • 217

_add_node I removed. It would make sencesense to insert at a specific index, but here it just confuses.

_add_node I removed. It would make sence to insert at a specific index, but here it just confuses.

_add_node I removed. It would make sense to insert at a specific index, but here it just confuses.

deleted 5 characters in body
Source Link
Joop Eggen
  • 4.7k
  • 15
  • 19
void append(LinkedList* list, int item){
    Node* p_node = malloc(sizeof(Node));
    if(p_node == NULL){
        printf("Memory allocation failed.");
        return;
    }
    p_node->data = item;
    p_node->next = NULL;
    if (list->head == NULL){
        list->head = p_head_node;p_node;
    }else{
        Node* cur = list->head;
        while (cur->next != null)
            cur = cur->next;
        cur->next = p_node;
    }
    list->length++;
}

int get(LinkedList* list, int index){
    index = check_valid_index(list, index);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    return cur.data;
}

void set(LinkedList* list, int at, int item){
    int index = check_valid_index(list, at);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    cur->data = item;
}
void append(LinkedList* list, int item){
    Node* p_node = malloc(sizeof(Node));
    if(p_node == NULL){
        printf("Memory allocation failed.");
        return;
    }
    p_node->data = item;
    p_node->next = NULL;
    if (list->head == NULL){
        list->head = p_head_node;
    }else{
        Node* cur = list->head;
        while (cur->next != null)
            cur = cur->next;
        cur->next = p_node;
    }
    list->length++;
}

int get(LinkedList* list, int index){
    index = check_valid_index(list, index);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    return cur.data;
}

void set(LinkedList* list, int at, int item){
    int index = check_valid_index(list, at);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    cur->data = item;
}
void append(LinkedList* list, int item){
    Node* p_node = malloc(sizeof(Node));
    if(p_node == NULL){
        printf("Memory allocation failed.");
        return;
    }
    p_node->data = item;
    p_node->next = NULL;
    if (list->head == NULL){
        list->head = p_node;
    }else{
        Node* cur = list->head;
        while (cur->next != null)
            cur = cur->next;
        cur->next = p_node;
    }
    list->length++;
}

int get(LinkedList* list, int index){
    index = check_valid_index(list, index);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    return cur.data;
}

void set(LinkedList* list, int at, int item){
    int index = check_valid_index(list, at);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    cur->data = item;
}
Source Link
Joop Eggen
  • 4.7k
  • 15
  • 19

(First: my personal preference is to use considerable more spaces and braces, but I'll keep to your style.)

The field index might be nice during development, but has no use.

typedef struct Node {
    struct Node* next;
    int data;
} Node;

If not exit, at least stop malloc from wreaking havoc.

LinkedList* new_linked_list(){
    LinkedList* list = malloc(sizeof(LinkedList));
    if(list == NULL) {
        printf("Memory allocation failed.");
        return NULL;
    }
    list->head = NULL;
    list->length = 0;
    return list;
}

_add_node I removed. It would make sence to insert at a specific index, but here it just confuses.

void append(LinkedList* list, int item){
    Node* p_node = malloc(sizeof(Node));
    if(p_node == NULL){
        printf("Memory allocation failed.");
        return;
    }
    p_node->data = item;
    p_node->next = NULL;
    if (list->head == NULL){
        list->head = p_head_node;
    }else{
        Node* cur = list->head;
        while (cur->next != null)
            cur = cur->next;
        cur->next = p_node;
    }
    list->length++;
}

int get(LinkedList* list, int index){
    index = check_valid_index(list, index);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    return cur.data;
}

void set(LinkedList* list, int at, int item){
    int index = check_valid_index(list, at);
    Node cur = list->head;
    while(index-- > 0)
        cur = cur->next;
    cur->data = item;
}

Sorting was super inefficient as you deal with a linked list (set, get). A better way would use pointers Node*.

void sort(LinkedList* list){
    if(list->length <= 1)
        return;
    Node* cur = list->head;
    while (cur->next != NULL){
        int min = cur->data;
        Node* next = cur->next;
        while (next != NULL) {
            if (next.data < min){
                int new_min = next.data;
                next.data = min;
                min = new_min;
            }
            next = next->next;
        }
        cur.data = min;
        cur = cur->next;
    }
}

Sorting here is done switching the data field values between nodes. There also exists the more difficult method of switching Node* pointers. There exist better sorting algorithms in C. Here you need N²/2 steps: O(N²). One can achieve O(N log N).

Checking the valid index is a good idea, however here it a backwards indexing too. Maybe change the name.

int check_valid_index(LinkedList* list, int index){
    // Negative index:
    if(index < 0)
        index = list->length + index;
    
    if(index >= list->length || index < 0){
        printf("Error: index %d is out of bounds for LinkedList of size %d\n", index, list->length);
        exit(1);
    }  
    return index;
}

The naming of pop is misleading. It is meant to remove the last item. However with push and pop one would here (linked list) think of the easy operations of adding/removing an item at the head.

In pop you free a NULL list->head.

This is a fine place to use aliases (Node**) the address of either list->head or some next field. This allows elegant code without ->next->next.

int pop(LinkedList* list){
    Node** current = &(list->head);
    if(*current == NULL){
        printf("Can't pop from an empty list\n");
        exit(1);
    }
    while((*current)->next != NULL)
        current = &(*current)->next;
    
    int value = (*current)->data;
    
    // Free the tail:
    free(*current);
    *current = NULL;
    list->length--;
    return value;
}

Using Node instead of Node* for walking through the list has the disadvantage of copying the Node struct every time. For more complex structs this is not good style.

void print_linked_list(LinkedList* list){   
    printf("{");
    Node* current = list->head;
    while(current != NULL){
        printf("%d", current->data);
        current = current->next;
        if (current != null)
            puts(", ");
    }
    printf("}\n");
}

Freeing is straight-forward.

void free_linked_list(LinkedList* list){
    Node* current = list->head;
    while(current != NULL){
        Node* node_to_be_freed = current;
        current = current->next;
        free(node_to_be_freed);
    }   
    free(list);
}

In general the order of execution at some points were reversed.