リンクリスト、deleteRecord() の書き方は？

質問

以下のような関数がありまして、
リストの真ん中と最後を削除することは
できるのですが 先頭がうまくいきません。

また、先頭の場合は free() をどのように使うのでしょうか？

code を見直してみると読み易くないような気もするんですが、
もっと効率の良い書き方ありますでしょうか？

int deleteRecord(struct record **start, char name[])
{
　　int judge = 0;
　　struct record *temp, *temp2;
　　temp = *start;

　　if(debugmode == 1)
　　　　printf("deleteRecord called.\n");

　　if(temp != NULL) {
　　　　if(strcmp(temp->name, name) == 0) {
　　　　　　if(temp->next == NULL) {　/* only start point */
　　　　　　　　temp = NULL;
　　　　　　}
　　　　　　else {
　　　　　　　　temp = temp->next;
　　　　　　}
　　　　　　judge = 1;
　　　　}
　　　　else if(temp->next != NULL) {
　　　　　　while(temp->next != NULL) {
　　　　　　　　if(strcmp(temp->next->name, name) == 0) {
　　　　　　　　　　if(temp->next->next != NULL) {
　　　　　　　　　　　　free(temp->next);
　　　　　　　　　　　　temp->next = temp->next->next;
　　　　　　　　　　}
　　　　　　　　　　else {
　　　　　　　　　　　　free(temp->next);
　　　　　　　　　　　　temp->next = NULL;
　　　　　　　　　　}
　　　　　　　　　　judge = 1;
　　　　　　　　}
　　　　　　　　if(temp->next != NULL)
　　　　　　　　　　temp = temp->next;
　　　　　　}
　　　　}
　　}
　　return judge;
}

回答No.4

sakusaker7 · ベストアンサー

同じstrcmpを三箇所でやってるのはよくないですね。
DRY(Don't Repeat Yourself)でいきましょう。

即興で書いたんで多分バグがいますし、もうちょっと書き直せますがとりあえず。
#一回無駄な strcmpが入るのはご愛嬌。工夫すれば避けられますけどね。

struct record {
　　char name[16];
　　struct record *next;
};

define strEQ(x, y) (strcmp((x), (y)) == 0)

int
delete_record2(struct record **start, char name[])
{
　　struct record *p;
　　struct record head;
　　int judge = 0;

　　//if (!*start)
　　//　　return judge;

　　head.name[0] = '\0';
　　head.next = *start;

　　p = &head;

　　while (p) {
　　　　if (strEQ(p->name, name)) {
　　　　　　struct record *tmp;

　　　　　　tmp = p;
　　　　　　p = p->next;
　　　　　　free(tmp);
　　　　　　judge = 1;
　　　　　　continue;
　　　　}
　　　　p = p->next;
　　}

　　return judge;
}

あと気になる点を。
同じ値を持つものは重複登録される可能性があり、削除のときは
同じ値を持つものはすべて削除というのは管理のやり方として
どうかなという気がします。
#リストを全部なめないといけないから
まあ登録時に重複登録をチェックしても同様の手間になりますが。

お礼

＞　削除のときは同じ値を持つものはすべて削除というのは
管理のやり方としてどうかなという気がします。

それがこの課題の「仕様」なんで今回はこれで良いんです^^
DRY なんて法則があるんですね。 とても勉強になりました。
丁寧に教えていただき ほんとにありがとうございます！！

回答No.3

Tacosan

とりあえず, 「レコードを検索する」関数と「レコードを削除する関数」を分けた方がいいんじゃないかなぁ. 例えば検索する関数を
struct record **findRecord(struct record **start, const char name[]);
削除する関数を
int deleteRecord(struct record **item);
と宣言しておく, みたいな.
deleteRecord の中身は難しくなく, 基本的には
if (item != NULL && *item != NULL) {
struct record *temp = *item;
*item = temp->next;
killRecord(temp);
}
で終わり. あ, killRecord は「そのレコードのデータを (必要なら中身の free も行って) 全部削除して, そのレコードそのものも free する」関数.

回答No.2

ape5

先頭については、*start != NULL なので

if(temp!=NULL){
　if(strcmp(temp->name,name)==0){
　　temp2=temp->next;
　　start = &temp2;
　　free(temp);
　　temp=temp2;
　}else
　　・・・・・・・・
}

コードは何度も推敲しているうちに単純化できるところに気づいていきます。
がんばってください。

回答No.1

sakusaker7

とりあえずヒントだけ。

リストの先頭に、実データ(この例でいえば name)を持たないリンクだけの
「いけにえ」を置くようにすれば、リストの(実際の)先頭要素を特別扱いしないで済むようにできます。

ところで

>　free(temp->next);
>　temp->next = temp->next->next;

temp->next を free しているのに、その後で temp->next->next
なんてアクセスしてはダメです。環境によっては動くかもしれませんが、
動かなくても文句は言えません。

補足

返事が遅くなり申し訳ないです。
みなさんのヒントを参考に、ようやく一通り
動くものが出来たのですが どうでしょうか？

int deleteRecord(struct record **start, char name[])
{　　　　　　　　　　　　
　　　　struct record *temp, *temp2;
　　　　int judge = 0;　

　　　　temp = temp2 = *start;

　　　　if(debugmode == 1)
　　　　　　　　printf("[ deleteRecord called. ]\n\n");

　　　　if(*start != NULL) {
　　　　　　　　while((temp->next != NULL) && (strcmp((*start)->name, name) == 0)) {
　　　　　　　　　　　　*start = (*start)->next;
　　　　　　　　　　　　free(temp);
　　　　　　　　　　　　temp = temp2 = *start;
　　　　　　　　　　　　judge = 1;
　　　　　　　　}　　　　　　　
　　　　　　　　while(temp->next != NULL) {
　　　　　　　　　　　　if(strcmp(temp->name, name) == 0) {
　　　　　　　　　　　　　　　　temp2->next = temp->next;
　　　　　　　　　　　　　　　　free(temp);
　　　　　　　　　　　　　　　　temp = temp2;
　　　　　　　　　　　　　　　　judge = 1;
　　　　　　　　　　　　}
　　　　　　　　　　　　temp2 = temp;
　　　　　　　　　　　　temp = temp->next;
　　　　　　　　}
　　　　　　　　if(strcmp(temp->name, name) == 0) { /* if only one list */
　　　　　　　　　　　　temp2->next = NULL;
　　　　　　　　　　　　free(temp);
　　　　　　　　　　　　judge = 1;

　　　　　　　　　　　　if(*start == temp)
　　　　　　　　　　　　　　　　*start = NULL;
　　　　　　　　}
　　　　}
　　　　return judge;
}