在stackexchange有一个人提了这么一个问题，自己实现了一个通用队列，然后把代码贴了上来，然后请大家review以下，希望基于以下几方面提一些建议：
- 1，编程风格(My general style）
- 2，是否正确地释放内存以避免内存泄漏
- 3，正确地处理了内存分配失败
- 4，其它任何你能想得到的建议
在我看来，下面的实现其实已经相当完备了，代码正确性，风格，可读性都相当不错，可是下面还有一人竟然提出了12条改进建议，让我觉得相当惊讶，我看了下这个人的简介，coding since 1976，让我觉得一个有着多年经验的程序员和普通程序员之间的差别。
代码如下：
Queue.h
#ifndef QUEUE_H_INCLUDED
#define QUEUE_H_INCLUDED
typedef struct Node
{
void *data;
struct Node *next;
}node;
typedef struct QueueList
{
int sizeOfQueue;
size_t memSize;
node *head;
node *tail;
}Queue;
void queueInit(Queue *q, size_t memSize);
int enqueue(Queue *, const void *);
void dequeue(Queue *, void *);
void queuePeek(Queue *, void *);
void clearQueue(Queue *);
int getQueueSize(Queue *);
#endif /* QUEUE_H_INCLUDED */
Queue.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "Queue.h"
void queueInit(Queue *q, size_t memSize)
{
q->sizeOfQueue = 0;
q->memSize = memSize;
q->head = q->tail = NULL;
}
int enqueue(Queue *q, const void *data)
{
node *newNode = (node *)malloc(sizeof(node));
if(newNode == NULL)
{
return -1;
}
newNode->data = malloc(q->memSize);
if(newNode->data == NULL)
{
free(newNode);
return -1;
}
newNode->next = NULL;
memcpy(newNode->data, data, q->memSize);
if(q->sizeOfQueue == 0)
{
q->head = q->tail = newNode;
}
else
{
q->tail->next = newNode;
q->tail = newNode;
}
q->sizeOfQueue++;
return 0;
}
void dequeue(Queue *q, void *data)
{
if(q->sizeOfQueue > 0)
{
node *temp = q->head;
memcpy(data, temp->data, q->memSize);
if(q->sizeOfQueue > 1)
{
q->head = q->head->next;
}
else
{
q->head = NULL;
q->tail = NULL;
}
q->sizeOfQueue--;
free(temp->data);
free(temp);
}
}
void queuePeek(Queue *q, void *data)
{
if(q->sizeOfQueue > 0)
{
node *temp = q->head;
memcpy(data, temp->data, q->memSize);
}
}
void clearQueue(Queue *q)
{
node *temp;
while(q->sizeOfQueue > 0)
{
temp = q->head;
q->head = temp->next;
free(temp->data);
free(temp);
q->sizeOfQueue--;
}
q->head = q->tail = NULL;
}
int getQueueSize(Queue *q)
{
return q->sizeOfQueue;
}
TestQueue.c
#include <stdio.h>
#include <stdlib.h>
#include "Queue.h"
int main()
{
int val;
Queue q;
queueInit(&q, sizeof(int));
for(val = 0; val < 10; val++)
{
enqueue(&q, &val);
printf("The value %d has been enqueued.\n", val + 1);
}
printf("\n");
queuePeek(&q, &val);
printf("The value that is at the front of the queue is %d\n\n", val + 1);
while(getQueueSize(&q) > 0)
{
dequeue(&q, &val);
printf("%d has been dequeued.\n", val + 1);
}
return 0;
}
下面让我看看他提的一些建议：
- 1，Pattern-less function names。对于queue.h里面一些函数的命名， Queue_Init(), Queue_GetSize() 要比 queueInit(), getQueueSize() 更能凸显意义，更具有一致性。
- 2，Comments with # preprocessing may not be portable。注释带了#可能移植性不太好。这个没理解，原程序中似乎也没有这样用注释，哪位读者明白可以评论里面补充下。
// #endif /* QUEUE_H_INCLUDED */
#endif
- 3，QueueList没有必要保存两个head和tail两个指针，只需要保存tail指针，并且tail结点的next指针域指向头结点，这实际上是一个循环链表组成的queue，那么链表走到末端的判断将会是p->next == tail->next。这样的好处是大量使用queue的时候更节省内存，但是逻辑上复杂了些。
- 4 不够明晰为什么队列大小类型使用int表示。int是（有符号）signed类型，可能表示负数，也许可以使用unsigned，在大多数系统中，size_t表示的范围要比int更加宽广，也许使用size_t更加谨慎一些。健壮的代码应该在enqueue函数中检查进队时是否超过队列最大长度。
- 5 好的点：声明memSize为size_t类型，对于malloc()函数的错误检查，有测试用例。另外对于.h的使用有注释说明，对于你的代码来说.h就像是一个公共借口。
关于size_t，int和unsigned int可参考此文https://prateekvjoshi.com/2015/01/03/why-do-we-need-size_t/, size_t,size_t的取值range是目标平台下最大可能的数组尺寸（非负数）,一些平台下size_t的范围小于int的正数范围,又或者大于unsigned int。为了移植性和性能考虑，有些情况下表示大小尽可能使用size_t
- 6 更加健壮的用法在queueInit()函数中检查memSize==0，同样对于malloc()函数应该做以下检查：if(newNode->data == NULL && q->memSize > 0)
- 7 要有些文档化的解释。对于queue.h每个函数声明前面建议由一到两行的注释。
- 8 对于queuePeek(Queue *q, void data)函数，如果q不改变，则应该声明为queuePeek(const Queue *q, void *data)。这是对于q常量特性的自我文档说明，可能对编译器优化也有好处。另外对于函数的使用实现检查也是有好处。
- 9 为了完备性，建议在clearQueue()函数中写上q->memSize = 0 。
- 10 改变#include的包含顺序。把"queue.h"应该放在第一个。这样可以检测到queue.h没有依赖下面的三个<.h>文件。queue.h需要依赖的已经在其本身文件中包含进来。
#include "Queue.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
// #include "Queue.h"
关于头文件依赖顺序说明可以参看：https://zh-google-styleguide.readthedocs.io/en/latest/google-cpp-styleguide/headers/，
1，foo/bar.h.
2，C 系统文件
3，C++系统文件
4，其他库头文件
5，本项目内头文件,
实际上《C++编程思想》对于2,3,4,5建议的顺序可能是反过来的，两者各有利弊。但有一点本文件相关的头文件必须放在第一位，可以防止在本文件（如上面的bar.h）里面少包含了必须的头文件。
- 11 结果声明。对于void dequeue(Queue *q, void *data)函数中拷贝data的操作，没有对结果的声明，应该失败返回fasle，成功返回true。queuePeek()函数也类似。
- 12 为了调试方便，释放内存操作free()之前最好把内存数据置零，这样错误的代码可能在面对原始数据对比为0的指针或者数据更快失败，更容易调试。
- 13 建议：如果非必要，可以不用存储queue的size大小，在需要的时候计算即可。也许可以添加一个检查是否为空队列的函数bool Queue_IsEmpty(const Queue *q)。这样可以去掉size的声明。
另外还有一个回答建议Node结构体的声明可以在.c文件中定义，如果其不是对外公开的数据结构体。
原文网址：
https://codereview.stackexchange.com/questions/141238/implementing-a-generic-queue-in-c
网友评论