Is this considerably good way to insert data at given position in doubly Linked List?

Question

Can this InsertAt() function thing be done in more better way?

I am in need for good guide for computer programing who can help me improve my coding skills in 1 to 1 manner. i wrote this line because my question was not get submitted due to too little details ,because i had nothing more to say. This is little anoying thing in StackOverflow

code

#include <stdio.h>
#include <stdlib.h>
struct Node *Head;
struct Node
{
    char data;
    struct Node *prev;
    struct Node *next;
};
//this function inserts data at head.
void Insert(char data)
{
    struct Node *tempHead = (struct Node *)malloc(sizeof(struct Node));
    tempHead->data = data;
    tempHead->next = Head;
    tempHead->prev = NULL;
    if (Head != NULL)
        Head->prev = tempHead;
    Head = tempHead;
}
void Print()
{
    struct Node *tempHead = Head;
    while (tempHead != NULL)
    {
        printf("%c,", tempHead->data);
        tempHead = tempHead->next;
    }
    printf("\n");
}
// this function inserts at given pos .
void InsertAt(int pos)
{
    struct Node *tempHead = Head;
    struct Node *container = (struct Node *)malloc(sizeof(struct Node));
    container->data = 'k';
    if (pos == 1)
    {
        Head->prev = container;
        container->prev = NULL;
        container->next = Head;
        Head = container;
        return;
    }
    else
    {
        for (int i = 1; i < pos; i++)
        {
            if (tempHead->next == NULL && i != (pos - 1))
            {
                printf("Data OverIndexed at  %d .\n",pos);
                break;
            }
            else if (tempHead->next == NULL && i == pos - 1)
            {
                tempHead->next = container;
                container->prev = tempHead;
                container->next = NULL;
                break;
            }

            else if (i == pos - 1)
            {
                struct Node *temp1 = tempHead->next;
                tempHead->next = container;
                container->prev = tempHead;
                container->next = temp1;
                temp1->prev = container;
                break;
            }

            tempHead = tempHead->next;
        }
    }
  
}

int main()
{
    Head = NULL;
    Insert('a');
    Insert('b');
    Insert('c');
    Insert('d');
    Insert('e');
    Insert('f');
      Print();
    InsertAt(1);
      Print();
    InsertAt(5);
      Print();
    InsertAt(12);
      Print();
    InsertAt(2);
      Print();
    InsertAt(20);
}

output

f,e,d,c,b,a,
k,f,e,d,c,b,a,
k,f,e,d,k,c,b,a,
Data OverIndexed at  12 .
k,f,e,d,k,c,b,a,
k,k,f,e,d,k,c,b,a,
Data OverIndexed at  20 .

Answer 1

Can this InsertAt() function thing be done in more better way?

Yes, it can. There are several bugs in your code.

Try this:

int main()
{
    Head = NULL;
    InsertAt(1);
    Print();
}

Did it crash?

You dereference Head without checking for NULL here:

if (pos == 1)
{
    Head->prev = container;  <--- what if Head is NULL ?

Another bug:

If the function InsertAt is called with pos being <= 0, the function silently returns, i.e. no error printed like done when pos is too high.

Yet another bug:

In the cases where you can't insert the new element, i.e. pos <= 0 or pos > length-of-list, you leak memory. You have already used malloc to get a new node but when the insert doesn't happen, the memory is "lost". Don't malloc before knowing whether you need the new node.

Besides that:

1. It's a real bad idea to use a global Head

2. A function that can fail (to insert the new node) should have a return value to indicate succes/failure

3. It's very strange that InsertAt doesn't take a datavalue (like Insert).

I would do something like:

int InsertAt(struct Node **pHead, int pos, char data)
{
    if (pos <= 0) return -1;   // Illegal pos

    if (pos == 1)
    {
        // pos == 1 is always valid
        struct Node *container = malloc(sizeof *container);
        if (container == NULL) exit(1);
        container->data = data;
        container->prev = NULL;
        container->next = *pHead;
        if (container->next != NULL) container->next->prev = container;
        *pHead = container;  // Change Head
        return 0;
    }

    if (*pHead == NULL) return -1;   // Illegal pos

    struct Node *tempHead = *pHead;
    for (int i = 1; i < (pos-1); i++)  // Notice pos-1
    {
        tempHead = tempHead->next;
        if (tempHead == NULL) return -1;  // Illegal pos
    }

    // Insert after tempHead
    struct Node *container = malloc(sizeof *container);
    if (container == NULL) exit(1);
    container->data = data;
    container->prev = tempHead;
    container->next = tempHead->next;
    tempHead->next = container;
    if (container->next != NULL) container->next->prev = container;

    return 0;
}

And call like:

struct Node *Head = NULL;
if (InsertAt(&Head, 1, 'b')) puts("InsertAt failed");
if (InsertAt(&Head, 2, 'd')) puts("InsertAt failed");
if (InsertAt(&Head, 2, 'c')) puts("InsertAt failed");
if (InsertAt(&Head, 1, 'a')) puts("InsertAt failed");