Reading a given element from a queue in C code

Question

I've found this simple Queue code and I'm trying to change some stuff. Since it was in spanish, I translated hoping that you can understand.

#include <stdio.h>
#include <Windows.h>

/* Returns "a - b" in seconds */
double performancecounter_diff(LARGE_INTEGER *a, LARGE_INTEGER *b)
{
LARGE_INTEGER freq;
QueryPerformanceFrequency(&freq);
return (double)(a->QuadPart - b->QuadPart) / (double)freq.QuadPart;
}


typedef struct _nodo {
int value;
struct _nodo *next;
} TypeNodo;

typedef TypeNodo *pNodo;

/* Queues functions */
void Insert(pNodo *first, pNodo *last, int v);
int Seek(pNodo *first, pNodo *last, int v);

int main() {
LARGE_INTEGER t_ini, t_fin;
double secs;
QueryPerformanceCounter(&t_ini);
pNodo first = NULL, last = NULL;
int x = 1;
while (x <= 80)
{
    Insert(&first, &last, x);
    x++;
}
printf("%d", Seek(&first, &last,18));
printf("%d", Seek(&first, &last, 2));

QueryPerformanceCounter(&t_fin);
secs = performancecounter_diff(&t_fin, &t_ini);
printf("Algoritmo de manejo de brazo por FCFS: \n");
printf("%.16g milisegundos\n", secs * 1000.0);
system("pause");
return 0;
}

void Insert(pNodo *first, pNodo *last, int v) {
pNodo New;

/* Create a new nodo and allocate it */
New = (pNodo)malloc(sizeof(TypeNodo));
New->value = v;
/* This will be the last nodo and will point to NULL */
New->next = NULL;
/* If queue isn't empty, then add the new nodo next to the last one */
if (*last) (*last)->next = New;
/* Now, the last element of the queue is the new one */
*last = New;
/* If first is NULL, the queue is empty and the first will point to the new nodo,     too */
if (!*first) *first = New;
    }

    int Seek(pNodo *first, pNodo *last, int v) {
pNodo nodo, nodo_aux; /* Extra variable to manipulate the nodo */
int a;      /* Extra variable for return */

/* Nodo points to the first element of the queue */
nodo = *first;
nodo_aux = nodo;
if (!nodo) return 0; /* If no nodos in the queue, retunrs 0 */
while (*first != NULL)
{
    if (nodo->value == v)
    {
        /* Storing return value */
        a = nodo->value;
        return a;
    }
    /* Assign to the first nodo the second one address */
    a = *first = nodo->next;        
}   
/* Free the nodo */
free(nodo);
/* If queue is empty, last must be */
if (!*first) *last = NULL;
return NULL;
}

Note that the function below is the original and the one above is the one I'm trying to modify to Seek an element in the queue by giving it when I call the function.

//int Seek(pNodo *first, pNodo *last) {
//  pNodo nodo; /* variable auxiliar para manipular nodo */
//  int a;      /* variable auxiliar para retorno */
//
//  /* Nodo apunta al primer elemento de la fila */
//  nodo = *first;
//  if (!nodo) return 0; /* Si no hay nodos en la fila retornamos 0 */
//  /* Asignamos al primer nodo la dirección del segundo nodo */
//  *first = nodo->next;
//  /* Guardamos el value de retorno */
//  a = nodo->value;
//  /* Borrar el nodo */
//  free(nodo);
//  /* Si la cola quedó vacía, last debe ser NULL también */
//  if (!*first) *last = NULL;
//  return a;
//}

When I run the all the code, the console shows nothing. I don't know what I'm missing here in Seek(). Any help would be appreciated.

Answer 1

It's a lot simpler than you have.

int Seek(pNodo first, int v)
{
   while (first != NULL)
   {
      if (first->value == v)
      {
         return v;
      }
      first = first->next;        
   }   

   /* Didn't find the value */
   return 0;
}

A better alternative is to return the node that contains the value. It the value is not in the queue, return NULL. (Thanks to the suggestion by M Oehm)

pNode Seek(pNodo first, int v)
{
   while (first != NULL)
   {
      if (first->value == v)
      {
         return first;
      }
      first = first->next;        
   }   

   /* Didn't find the value */
   return NULL;
}

Answer 2

The original Seek is badly named: It deletes the first node in the queue and returns its value, updating the queue's head. (It should probalby be called Pop or something like this.)

Your Seek tries to find a node, but keeps the queue intact. That means:

• You don't need to pass a pointer to the head pointer, because you don't have to update it.
• You don't need to pass last, because that also never changes.
• Your return value should probably be the node pointer of the found node or NULL. You return the node's value, which you already know - you pass it as parameter.
• You shouldn't free anything here. (Without updating the other nodes' pointers, that's recipe for disaster, really.)
• You don't need auxiliary nodes. If you look at your code, you have two local node pointers and you mess them up. nodo_aux is never used, so remove it. Then you should decide whether you do your work with *first or with node. Your code uses both nearly interchangeably, which corrupts your logic. For example, you always update first, but based on nodo, which never changes.
• Then you even mix up integers and pointers, which isn't a good idea. If you look at the code, you don't really need the intermediary variable a and the assignment a = *first is useless anyway, because you never do anything with that assigned value of a.

Look at R Sahu's answer for a clean implementation of Seek.