Link list working

Question

I wrote this singly linked list and it worked fine,Now can anyone review my code. I want to do some more clean my delete and add function. And some abstraction.

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

typedef struct q_9
{
   int data;
   struct q_9 *link;
}*node;

node head=NULL;
node tail=NULL;

node newnode(int x);
void node_add(int x);
void delete_front();
void count();
void display();
void delete_last();

int main() {

   int choice,element;


   while (1)
   {
       printf("\n\t 1-->for add data:");
       printf("\n\t 2-->for delete data:");
       printf("\n\t 3-->for count node:");
       printf("\n\t 4-->for delete from last data:");
       printf("\n\t 5-->for display data:");
       printf("\n please enter a choice:");
       scanf("%d",&choice);
       switch (choice)
       {

           case 1: printf("enter the element:\t------->");
                   scanf("%d",&element);
                   node_add(element);
                   break;
           case 2:delete_front();
                   break;
           case 3: count();
                   break;    
           case 4: delete_last();
                   break;              
           case 5:display();
                   break;      
           default:
                   break;
       }

       if (choice==6)
       {
           printf("you are exited from loop:\n");
           break;
       }
   }
}

node newnode(int x)
{
   node temp=(node)malloc(sizeof(node));
   temp->data=x;
   temp->link=NULL;
   return temp;
}

void node_add(int x)
{
   node temp=newnode(x);

   if(head==NULL)
   {
       head=temp;
       tail=temp;
       temp=NULL;
       return;
   }
   else
   {
       tail->link=temp;
       tail=temp;
   }
}

void display()
{
   node p=head;
   while (p!=NULL)
   {
       printf("%d\t",p->data);
       p=p->link;
   }
}

void count()
{
   node p=head;
   if (p==NULL)
   {
       printf("no element present here\n");
   }

   int i=0;
   while(p!=NULL)
   {
       i++;
       p=p->link;
   }

   printf("here is %d nodes are present\n",i);
}

void delete_front()
{
  node temp=head;
  head=head->link;
  free(temp);
  printf("\n memory freed");
}

void delete_last()
{
   node temp=tail;
   node p=head;

   while(p->link->link!=NULL)p=p->link;

   tail=p->link;

   free(temp);
   printf("memory freed\n");
}

Answer 1

Design: OK initial attempt at abstraction

main() does not expose struct q_9 members.

Design: .tail is not needed

Present function set has no benefit from the tail member. It is removable. Even delete_last() walks down the list.

Design: Global variables not needed

This is a major change, so will assume this code is using the simple one linked-list as an exercise.

Design: Consider common naming prefix (and avoid type-defining a pointer)

Example:

typedef struct q9 {
  struct q9 *link;
  int data;
} q9_T;

q9_T *q9_newnode(int x);
int   q9_count(const q9_T *node);
void  q9_display(const q9_T *node);
void  q9_add(q9_T *node, int x);
void  q9_delete_front(q9_T *node);
void  q9_delete_last(q9_T *node);
// new ideas
int   q9_pop(q9_T *node);
int   q9_top(const q9_T *node);
int   q9_end(const q9_T *node);  // Potential real use of .tail
bool  q9_empty(const q9_T *node);

Bug: Wrong allocation size

Code allocates enough for a pointer rather than enough for the struct.

Cast not needed. Better to allocate to the de-referenced object than type.

// node temp=(node)malloc(sizeof(node));
node temp = malloc(sizeof *temp);

Function declarations say nothing about signature

void foo(); allows later calls like foo(1), foo("hello", 5.0) to pass undetected as errors.

// void delete_front();
// void count();
// void display();
// void delete_last();
void delete_front(void);
void count(void);
void display(void);
void delete_last(void);

Inconsistent spacing

Use an auto-formatter - life is too short for manual formatting.

Lack of error checking

No checking if scanf("%d",&choice); succeeded, nor malloc().

Unneeded code

temp=NULL; serves nor purpose in node_add(int x)

Bug: de-referencing of NULL

delete_front() does not check if head == NULL before head->link. tail is not updated to NULL when last link deleted.

Bug: de-referencing of NULL

delete_last() does not check if p->link == NULL before p->link->link. Also expect head to be updated to NULL if last node deleted.

Prompts use lower case rather than sentence case.

Unless you are E. E. Cummings, use sentence case for prompts.

// printf("\n please enter a choice:");
printf("\n Please enter a choice:");

Insure output is seen before input

Due to various buffer modes of stdout, best to flush output before input, especially if the prior output does not in with a '\n'.

   printf("\n please enter a choice:");
   fflush(stdout); // add
   scanf("%d",&choice);

Answer 2

If one wants abstraction, then the interface (the menu) should be in a different file then the linked-list implementation, and should expose only function prototypes which are necessary in a header file, (the rest can be static.) The one with main should drive your linked list; the linked list should have no main. That way, code would be interchangeable into other, more complicated programmes. In practice, one should do an automated test in another file to see if the, edge cases in particular, (eg, empty list, which is not robust,) do what one expects them to.

I would question this use as a singleton, but maybe it's the way it's supposed to work. To break out of this pattern, I would accept another parameter to all functions, (except the constructor, in this case newnode.) In a sense, a linked-list is a tricky thing to abstract because, by definition, it's a recursive data structure, so how can one tell the middle of a list from the head? I suggest the looking at Wikipedia, C++, Java, and C# for existing implementations.

One should turn up the warnings on ones compiler: in particular, #include <stdio.h> is necessary for printf. (However, one should consider not using this function to make it more abstract.)

Beware of the effects of typedef on namespace; the Linux kernel coding style would frown upon such usage. In particular, the naming of q_9 and node is problematic when one gets into more complex programmes. Maybe struct int_node or even struct int_list_node?

When using, eg, int main(), beware that one is not using prototypes and subverting the compiler's ability to type-check. One should really use int main(void).

One doesn't check the return value of malloc. This is bad because the function can return null and then the code has temp->null=x, which is potentially dereferencing a null pointer.

For example, void count() is fine for printing out the lists count on stdout, but maybe this should be int count(void), (or even size_t count(void),) the user, (of the abstract file, probably yourself,) should be able to decide what they want to do with this information. Same with the print memory freed, one doesn't want to do this all the time.