Relocating a Node in a Singly Linked List Creates Infinite Loop

Question

In my homework I was asked to relocate a specific node by his name field to specific index. I'm having trouble to figure out why is my list becomes an infinite list. The problem is in the function "changePlacement"
Example:
Given linked list : 1->2->3->4
Output : 1->3->3->3...
Expected output : 1->3->2->4

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

struct Frame
{
    char            *name;
    unsigned int    duration;
    char            *path;  // may change to FILE*
};
typedef struct Frame frame_t;

struct Link
{
    frame_t *frame;
    struct Link *next;
};
typedef struct Link link_t;

int listLen = 0;
link_t* lastNode = NULL;

void addFrame(link_t** list);
void frameData(frame_t* frame);
void freeVideo(link_t* video);
void showList(link_t* list);
void freeNode(link_t* list);
void changePlacement(link_t** list, int newIndex, char* name);

int main(void)
{
    link_t* video = NULL;
    lastNode = video;
    if (!video)
    {
        perror(video);
    }
    addFrame(&video);
    addFrame(&video);
    addFrame(&video);
    addFrame(&video);

    showList(video);
    changePlacement(&video, 2, "3");
    showList(video);
    printf("\n");
    freeVideo(&video);

    _flushall();
    getchar();
    return 0;
}



void addFrame( link_t** video)
{
    char strTemp[200] = { 0 };

    link_t* newFrame = (link_t*) calloc(1, sizeof(link_t));
    newFrame->frame = (frame_t*) malloc(sizeof(frame_t));

    printf("                  *** Creating new frame ***\n");
    printf("Please insert a frame path:\n");
    scanf(" %s", &strTemp);
    newFrame->frame->path = (char*) calloc(strnlen(strTemp, 200) + 1, sizeof(char));
    strncpy(newFrame->frame->path, strTemp, strnlen(strTemp, 200));

    printf("Please insert frame duration(in miliseconds):\n");
    scanf(" %d", &(newFrame->frame->duration));

    printf("Please choose a name for that frame:\n");
    scanf(" %s", &strTemp);
    newFrame->frame->name = (char*) calloc(strnlen(strTemp, 200) + 1, sizeof(char));
    strncpy(newFrame->frame->name, strTemp, strnlen(strTemp, 200));

    if (!(*video))
    {
        (*video) = newFrame;
    }
    else
    {
        lastNode->next = newFrame;
    }
    lastNode = newFrame;


    ++listLen;
}




void freeVideo(link_t** video)
{
    link_t* currNode = NULL;
    int loop = 0;

    for (; currNode; currNode = *video) // set curr to head, stop if list empty.
    {
        // advance head to next element.
        freeNode(currNode);
        (*video) = (*video)->next; // delete saved pointer.
    }
    freeNode((*video)->next);
    freeNode((*video));


}


void showList(link_t* list)
{
    printf("\n\t|Name\t\t|    Duration   |     Path\n");
    printf("\t+++++++++++++++++++++++++++++++++++++++++++++++\n");
    for (;list; list = list->next)
    {
        printf("\t|%10s\t|%10d\t|\t%s\n", list->frame->name, list->frame->duration, list->frame->path);
        printf("\t+++++++++++++++++++++++++++++++++++++++++++++++\n");
    }
}

void freeNode(link_t* node)
{
    free(node->frame->name);
    free(node->frame->path);
    free(node->frame);
    free(node);
}

void changePlacement(link_t** list, int newIndex, char* name)
{
    link_t *prevOldNode = (*list), *prevTemp = NULL, *oldNode = NULL, *newNode = NULL;
    link_t *temp = (*list), *PrevNewNode = NULL, *nodeAfterNewNode = NULL;
    int currIndex = 0, i = 0, flag = 0;

    while ((temp->next))
    {
        ++currIndex;
        if (!strcmp(temp->frame->name, name)) //looking for the wanted node
        {
            oldNode = temp;
            prevOldNode = prevTemp;
        }
        if (currIndex == newIndex)
        {
            PrevNewNode = prevTemp;
            newNode = temp;
        }
        prevTemp = temp;
        temp = temp->next;
    }
    nodeAfterNewNode = newNode->next; //temporal variable that stores the node after the new node
    prevOldNode->next = oldNode->next;
    newNode->next = oldNode;

    oldNode->next = nodeAfterNewNode;
}

Do you have any ideas after looking on my code? any help would be blessed

Start by fixing your warnings. Then post the code *in your question*. Links can die (and in fact, some actually count on it), making the question useless for future readers. — WhozCraig, Jun 22 '16 at 20:10
Great, but [the warnings (and actually, errors) still persist](http://pastebin.com/hzixMP7u). You need to turn up your warning levels to pedantic levels on your compiler and address *all* the issues identified. Even then there are some things that the compiler will *not* catch, the error in your `freeVideo`, for example, which due to incorrect initialization, will not free anything except the first two nodes. A **debugger** seems ideal for solving these issues, including your infinite loop, and frankly that's the next step after fixing your warnings/errors. — WhozCraig, Jun 22 '16 at 20:25
in general, the kind of problem you state would be solved in three steps. 1) find the desired node 2) removed the node from the linked list (but keep the pointer to the node) 3) insert the node at the desired location. (be sure to allow for first node, last node and similar edge cases. — user3629249, Jun 23 '16 at 03:44
why have two structs for a simple linked list. Just make a new field in the first struct that is `struct Frame *next;` and modify the code to match — user3629249, Jun 23 '16 at 03:46
when calling any of the heap memory allocation functions: 1) always check (!=NULL) the returned value to assure the operation was successful. 2) the returned value has type: `void*` so can be assigned to any pointer. Casting just clutters the code, making it more difficult to understand, debug, maintain. — user3629249, Jun 23 '16 at 03:50
when calling any of the `scanf()` family of functions: 1) always check the returned value (not the parameter value) to assure the operation was successful. 2) when using the '%s' format specifier, always use a max-length modifier so the user cannot overrun the input buffer. Note: the max-length modifier has to be one less than the actual length of the input field because `scanf()` will append a NUL byte to the input — user3629249, Jun 23 '16 at 03:54
when calling the `calloc()` function, the first parameter (a `size_t`) is the number of items to allocate and the second parameter (also a `size_t`) is the size of each item. Please read the man page for calloc, the apply that knowledge to all your calls to `calloc()` — user3629249, Jun 23 '16 at 03:58

score 0 · Answer 1 · answered Jul 15 '16 at 13:01

Your code at the end of changePlacement() doesn't account for the case of oldNode and newNode being adjacent to each other, i. e. e. g. nodeAfterNewNode = newNode->next being identical to oldNode. (Besides it doesn't account for the case of not finding the name.) It is far easier to just exchange the frame pointers; change

    nodeAfterNewNode = newNode->next; //temporal variable that stores the node after the new node
    prevOldNode->next = oldNode->next;
    newNode->next = oldNode;

    oldNode->next = nodeAfterNewNode;

to

    if (!oldNode) return;   // wanted node name not found
    frame_t *f = oldNode->frame;
    oldNode->frame = newNode->frame;
    newNode->frame = f;

Relocating a Node in a Singly Linked List Creates Infinite Loop

1 Answers1