このプログラムどこがおかしいですか？

質問

コンパイルはできますが実行できません…
どこがおかしいか分かる人教えてください。
このプログラムはファイルから名前　数学の点数、英語の点数を読み込んで文字データに入れた後構造体に入れて探し出す値の項目（キー）を入力して（何番目にあるか）探し出すというプログラムです。

#include <stdio.h>
#include <string.h>
#define NUMBER 10
//構造体を宣言する
struct student
{
//名前、身長、体重を構造体オブジェクトのメンバに格納する関数の定義
char name[10];
char math[4];
char eng[4];
};

/*--- 要素数nの配列aからkeyと一致する要素を線形探索（番兵法） ---*/
int search(struct student *b, int n, char key)
{
int i=0;
b[i].name[0]=key; // 番兵を追加
while (1)
{
if (b[i].name[0] == key)
break; /* 見つけた */
i++;
}
return (i == n ? -1 : i);
}

int main(void)
{
FILE *fpin;
struct student a[NUMBER];
int i=0, idx,ret;
char buffer[20],ky;
int nx=sizeof(buffer) / sizeof(buffer[0]);

fpin=fopen("input2.txt","r"); //テキストファイルを読み取りモードで開く
while(fgets(&buffer[0],sizeof(buffer),fpin) !=NULL )
{
if(i>=100) break;//読み込む人数が100人を超えてる時の処理
ret=sscanf(&buffer[0],"%s %s %s",&a[i].name,&a[i].math,&a[i].eng); //データ文字列を3分割
if(ret!=3) //3に分割できなかったときの処理
{
puts("代入された入力項目の個数が3でありません");
goto END;
}
printf("%s %s %s\n",&a[i].name,&a[i].math,&a[i].eng);
i++;
}

printf("探す値：");
scanf("%s", &ky);

idx = search(a, nx - 1, ky); /* 配列xから値がkyである要素を線形探索 */

if (idx == -1)
puts("探索に失敗しました。");
else
printf("%dは%d番目にあります。\n", ky, idx + 1);
END: fclose(fpin);
return 0;
}

回答No.6

Wr5 · ベストアンサー

>オーバーランをなくすにはどうしたらいいですか？

#define NUMBER 10
を
#define NUMBER 100
にする。

または、
if(i>=100) break;//読み込む人数が100人を超えてる時の処理
を
if(i>=NUMBER) break;//読み込む人数が100人を超えてる時の処理
にする。(この場合、コメントがウソついていますが)

ちなみに、データいっぱいまで読み込んだ場合、
このアルゴリズムでは最終データが番兵により書き換えられてしまいます。
そういう意味では、#4さんの書かれているとおりサイズ渡しているので最終まで行ったら検索を中断するようにすればよいです。

お礼

ありがとうございます。
おかげで解決しました。

回答No.7

koko_u_

search() が名前の最初の 1文字でしか検索しないので、100人もの生徒を扱うのは無理だな。

あと、フラグはコーディングの瞬間は「わかりやすい」ですが、
次の瞬間には何のフラグだったのか忘れてしまうので、使わないに越したことはないと思います。

この場合で言えば、単純にキーを発見したら return で関数から抜ければいいだけでフラグは不要です。

回答No.5

tig33

バッファオーバーランになるのは、search()関数で、
検索するテーブルの大きさを考慮に入れていないからです。
指定されたものが見つからないときは、テーブルの最後まで
検索したら、検索を止めなくては行けませんね。

このテーブルの最後を検出するロジックが抜けているからです。

while(1) {
}

でループを作成すると、見つかったときは、ループを抜けますが、
見つからなかったときは、テーブルの範囲を超えて検索しようと
します。（大抵は、永久ループ）

したがって、for() 文を使った方が、簡明です。また、見つかったかどうかもフラグを用いた方が分かりやすいです。

int search( ... ) {
　　int found;

　　found = 0;
　　for(i=0; i < （テーブル個数）; i++) {
　　　　if(一致条件) {
　　　　　　found = 1;
　　　　}
　　}

　return( found ? i : -1 );
}
と言う具合ですね。

回答No.4

koko_u_

そもそも search() 関数には配列の要素数を渡しているんだから、番兵を立てる意味はないよね。

回答No.3

Wr5

連続書き込みすみません…

番兵でバッファオーバーランする前に…
> if(i>=100) break;//読み込む人数が100人を超えてる時の処理
>ret=sscanf(&buffer[0],"%s %s %s",&a[i].name,&a[i].math,&a[i].eng); //データ文字列を3分割

でオーバーランの可能性ありでした。
10行以上のデータを読み込ませた場合に…。

補足

連続書き込みありがとうございます。

オーバーランをなくすにはどうしたらいいですか？

回答No.2

Wr5

> 対して、searchに渡している最大値(おそらく第2引数)は
> > char buffer[20]
> > int nx=sizeof(buffer) / sizeof(buffer[0]);
> で20です。
> 番兵の位置を修正すると、a[19]の位置に書き込まれます。

「20です」って書いて、「a[19]」と書くと紛らわしいですね…

> idx = search(a, nx - 1, ky); /* 配列xから値がkyである要素を線形探索 */
で-1しているので、a[19]ということになります。

回答No.1

Wr5

まず、「実行できません」とはどういう状況になるのでしょうか？
補足願います。

で、
search()の戻り値は必ず0になるかと思われますが、
意図した動作ですか？
何故0になるのかは番兵が行列の先頭に居るから…ですが。

さらに、番兵の位置を修正したとしてもバッファオーバーランします。
> student a[NUMBER];
でNUMBERは10、a[0]～a[9]です。
対して、searchに渡している最大値(おそらく第2引数)は
> char buffer[20]
> int nx=sizeof(buffer) / sizeof(buffer[0]);
で20です。
番兵の位置を修正すると、a[19]の位置に書き込まれます。

補足

すみません
b[i].name[0]=key;でなく
b[n].name[0]=key;でした。
実行できないのは強制終了になる状況です。
バッファオーバーランをなくすには
NUMBERの値を大きくすればいいんですか？