4
\$\begingroup\$

I'm practicing data structures in C and I implemented a queue based on its description:

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

struct QueueNode {
    int data;
    struct QueueNode* nextNode;
};

struct Queue{
    struct QueueNode* priorNode;
    struct QueueNode* lastNode;
};

struct Queue* initQueue(){
    struct Queue* queue = malloc(sizeof(struct Queue)); //only one is needed
    if(!queue){
        fprintf(stderr,"memory error");
        exit(1);
    }
    queue->priorNode = NULL;
    queue->lastNode = NULL;
    return queue;
}

void enqueue(struct Queue* queue, int num){
    struct QueueNode* queuenode = malloc(sizeof(struct QueueNode));
    if(!queuenode){
        fprintf(stderr,"memory error");
        exit(1);
    }
    queuenode->data = num;
    queuenode->nextNode = NULL;

    if(!queue->priorNode){
        queue->priorNode = queuenode;
    }

    if(queue->lastNode){
        struct QueueNode* temp = queue->lastNode;
        queue->lastNode = queuenode;
        temp->nextNode = queue->lastNode;
    } else {
        queue->lastNode = queuenode;
    }
}

struct QueueNode dequeue(struct Queue* queue){
    if(!queue->priorNode){
        fprintf(stderr,"Queue is empty.\n");
        struct QueueNode nullnode = {0, NULL};
        return nullnode;
    }
    struct QueueNode prior = *queue->priorNode;
    free(queue->priorNode);
    if(prior.nextNode){
        queue->priorNode = prior.nextNode;
    } else {
        queue->priorNode = NULL;
    }
    return prior;
}

void peekQueue(struct Queue* queue){
    printf("\"%d\" is the current prior element in the queue\n", queue->priorNode->data);
}

void listQueue(struct Queue* queue){
    struct QueueNode* qn = queue->priorNode;

    printf("elements in the queue from left(the most prior) to right(the least prior) are:\n");
    while(qn){
        if(!qn->nextNode){
            printf("%d",qn->data);
            break;
        }
        else{
            printf("%d - ",qn->data);
            qn = qn->nextNode;
        }
    }
    printf("\n");
}

void closeQueue(struct Queue* queue){

    struct QueueNode* next = NULL;
    while(queue->priorNode){
        next = queue->priorNode->nextNode;
        free(queue->priorNode);
        if(!next) break;
        queue->priorNode = next;
    }

    free(queue);
}

int main(){
    struct Queue* queue = initQueue();

    enqueue(queue,15);
    enqueue(queue,4);
    enqueue(queue,8);
    enqueue(queue,13);
    enqueue(queue,7);

    listQueue(queue);

    peekQueue(queue);

    struct QueueNode node;

    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);
    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);
    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);
    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);
    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);
    node = dequeue(queue);
    printf("\"%d\" came out of the queue\n",node.data);

    closeQueue(queue);
return 0;
}

I especially didn't know what to do in my dequeue function when the queue was empty. I don't want the 0 to be returned as well because that would of course be confusing to the user.

\$\endgroup\$
1
\$\begingroup\$

I'm not able to review the functionality of the code currently, but to address your concerns:

In dequeue(), rather than returning the node, add a parameter (pointer to QueueNode) to store that node in. This is similar to functions like memcpy(3), strtoul(3) (end pointer), and so on. It allows you to return an error value however you wish.

int dequeue(struct Queue *queue, struct QueueNode *out);

If the queue is empty, don't bother modifying out, and just return an error code.

I would also avoid printing & exiting from within the queue's functions upon error. Instead return some kind of error value and leave it for the user to check.

For example, consider if malloc fails. The user may want to check errno, or use a fallback data structure.

int enqueue(struct Queue *queue, int num) {
    struct QueueNode *queuenode = malloc(sizeof(struct QueueNode));

    if(queuenode == NULL){
        return -1;
    }

    queuenode->data = num;
    queuenode->nextNode = NULL;

    if(!queue->priorNode){
        queue->priorNode = queuenode;
    }

    if(queue->lastNode){
        struct QueueNode* temp = queue->lastNode;
        queue->lastNode = queuenode;
        temp->nextNode = queue->lastNode;
    } else {
        queue->lastNode = queuenode;
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ thank you. if I use something like endptr, in strtoul, to where should I point it when my queue is empty? maybe i can add an int* errcode and set it to non-zero when an error occurs? also for exiting, errno is set anyway when error occurs so is another error needed from me? (i know exiting is wrong but i'm asking to realize what kind of error do you mean i should return). i exit to not allow next lines to run, so without an exit, how would that be possible? wrapping all next lines in an else statement? \$\endgroup\$ – aderchox Mar 1 at 5:52
  • 1
    \$\begingroup\$ I've updated my answer to clarify: add a parameter (out) which is a pointer that you can modify to point to the dequeued node. You should return the error, not as a pointer to an error value. A simple return -1 is usually sufficient for returning an integer error code. Even if errno is set by malloc, it's still best practice to return an error code -- this is used to indicate than an error occurred and the programmer should check errno. \$\endgroup\$ – esote Mar 1 at 6:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.