コードレビュー

引き続き押しかけコードレビュー

元コード

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

typedef struct LIST_T  list_t;
typedef struct LIST_ELEMENT_T  *elist_t;

/* リストノード構造体 */
struct LIST_ELEMENT_T  {
	int data;						/* 保持するデータ */
	struct LIST_ELEMENT_T *next;					/* nextポインタ */
};

/* リストを表す構造体 */
struct LIST_T  {
	struct LIST_ELEMENT_T  *head;	/* リストの先頭ノードを指す */
};

#if 0	/* C89用に書き直し */
list_t make_list()	{
	return	(list_t){ .head = NULL; }
}
#endif

void  add_list(list_t *ls, int data)  {
	elist_t e = (elist_t)malloc(sizeof(struct LIST_ELEMENT_T));
	e->next = ls->head;
	e->data = data;
	ls->head = e;
}

int sum_list(list_t ls)	 {
	int s = 0;
	elist_t it = ls.head;
	while(it)  {
		s += it->data;
		it = it->next;
	}
	return	s;
}

/* 使い方サンプルドライバ */
int main(void)	{
	list_t ls = {0};	/* C89対応のため置き換え make_list(); */
	int i;
	
	for(i = 1;	i <= 10;  i++)	{
		add_list(&ls,  i);
	}
	printf("%d\n",  sum_list(ls));
	return	0;
}

元のコードは複数ページに別れていたので、若干順番を入れ替えている部分もある。

stdlib.hのインクルード漏れ

typedef で定義した型名と元の型名が混在している。

typedefするからには置き換えた名前に寄せる方がよい。

typedefでポインタが隠蔽されている。

具体的には、typedef struct LIST_ELEMENT_T *elist_t;の部分。配列やポインタをtypedefすると宣言部分だけ見ると、配列やポインタには全く見えないのが個人的にはイヤ。命名則を決めて、型名でポインタや配列であることを主張すれば多少読みやすくなる。

リストを指す構造体を関数への渡し方のポリシーが不統一

具体的にはadd_listの第一引数はlist_t *なのに、sum_listの第一引数はlist_tになっている。そろえる方がよい。今回の場合はlist_tの大きさはたかがしれているので、統一してあればどちらでもよいかも。しかし巨大な構造体の場合はポインタを渡すことが多いので、ポインタを渡す方が好み。

gcc拡張(C99？)を使っている。

具体的にはmake_listの部分。今回の勉強会の性格では、C89の範囲に絞る方がよいのでは？C89に対応させるのは容易です。メンバを指定した初期化は、メンバ数や要素数が非常に多く、かつ値の設定が歯抜けになる場合にメリットが出ます。

コメントが全く入っていない。

コメントの入れ方にはセンスが必要です。ただし、スライド用にはコードが書きにくいからのような気がする。