改善点をアドバイスして下さい。

[taz]

こんにちは。以下プログラムですが、ここを改善した方がいいよという箇所はありますでしょうか？アドバイスよろしくお願いします。

コード:

//読み込みファイル
#include <stdio.h>
#include <stdlib.h>

#define MENU 0
#define ADDR 1
#define UPDATER 2
#define DELR 3
#define DISPR 4
#define CANCEL 5
#define NO 2
#define ERROR -1
#define NOERROR 0

FILE *fp; //ファイルポインタ
char *fname = "comma.csv"; //ファイル名
int ret = 0; //関数の返り値
int choice=0; //キーボード入力値格納変数
int i=0; //構造体配列の番号
int Pno;

//会員メンバ構造体
struct member 
{
	int no;
	char name[20];
	char adress[20];
	char tel[20];
	char mail[20];
};

struct member member[20]={0};//構造体メンバ宣言
struct member member1 = {EOF,EOF,EOF,EOF,EOF}; //構造体配列の終端値

/*メイン関数*/
main()
{
Pno = 0;
Pno = init_disp();
while(Pno!=CANCEL)
{
	switch(Pno)
	{
	case MENU:
		Pno = init_disp();
		break;
	case ADDR:
		Pno = add_record();
		break;
	case UPDATER:
		Pno = update_record();
		break;
	case DELR:
		Pno = del_record();
		break;
	}
}
}

/*新規登録*/
int add_record()
{
	printf("\n会員番号（0-10数値）？ ");
	scanf("%d",&(member[0].no)); //会員番号
	printf("会員名称（漢字,英語）？ ");
	scanf("%s",member[0].name); //名前
	printf("住所（漢字,数字50文字）？ ");
	scanf("%s",member[0].adress); //住所
	printf("電話番号（半角文字-で区切る）？ ");
	scanf("%s",member[0].tel); //電話番号
	printf("メールアドレス（半角英数字）？ ");
	scanf("%s",member[0].mail); //メールアドレス
	printf("新規登録しますか？（1:yes 2:no）？ ");
	scanf("%d",&choice); //番号の入力

	//新規登録しない時
	if(choice == NO)
	{
		system("cls"); //コマンドプロンプトの画面クリア
		return CANCEL; //メイン関数
	}

	fp = fopen(fname,"a"); //上書きモードでファイルを開く
	
	//ファイルポインタがNULLの時
	if(fp == NULL)
	{
		printf("%sファイルが開けません\n",fname);
		return ERROR; //エラー値として-1を返す
	}

	//ファイルへ出力する
	ret = csv_write(0);

	fclose(fp); //ファイルを閉じる

	printf("%sファイル書き込みが終わりました\n",fname);
	return NOERROR;
}

/*更新処理*/
int update_record()
{
	int no; //入力した会員番号
	int g=0; //構造体配列の番号
		
	printf("\n会員番号（0-10数値）？ ");
	scanf("%d",&no); //会員番号の入力

	fp = fopen(fname,"r"); //ファイルを読み込みモードでオープン
	
	//ファイルポインタがNULLの時
	if(fp == NULL)
	{
		printf("%sファイルが開けません\n",fname);
		return ERROR; //エラー返り値として-1を返す
	}
	
	//ファイル読み込み終了まで繰り返す
	while(feof(fp)==0)
	{
			//ファイルからデータを読み込む
			if((fscanf(fp,"%d,%[^,],%[^,],%[^,],%s",&(member[i].no),member[i].name,member[i].adress,member[i].tel,member[i].mail)) != EOF)
  			{
				//入力番号と配列の番号が等しい時
				if(no==member[i].no)
				{
				   printf("%s\n",member[i].name);
                   scanf("%s",member[i].name); //名前
                   printf("%s\n",member[i].adress);
                   scanf("%s",member[i].adress); //住所
                   printf("%s\n",member[i].tel);
                   scanf("%s",member[i].tel); //電話番号
                   printf("%s\n",member[i].mail);
                   scanf("%s",member[i].mail); //メールアドレス
                   printf("%s\n","変更登録しますか？（1:yes 2:no） ");
                   scanf("%d",&choice); //番号入力
				   
				   //変更登録しない場合
				   if(choice==NO)
				   {
						system("cls"); //コマンドプロンプトの画面をクリア
						return CANCEL; //メイン関数に戻る
				   }
				}
			}
			i++;
	}
	fclose(fp); //ファイルを閉じる
	fp = fopen(fname,"w"); //書き込みモードでオープンしファイルをクリア
	fclose(fp); //ファイルを閉じる
	fp = fopen(fname,"w"); //書き込みモードでファイルをオープン
	
	//配列の中身を全て取り出す
	while(g<i-1)
	{
		ret = csv_write(g);
		g++;
	}
	fclose(fp); //ファイルを閉じる
	printf("%s","ファイル書き込みが終わりました");
	return NOERROR;
}

/*削除処理*/
int del_record()
{
	int no; //会員番号の出力値
	printf("\n会員番号（0-10数値）？ ");
	scanf("%d",&no); //会員番号の入力
    ret = csv_read(); //ファイルの読み込み関数
	
	//ファイル読み込み終了まで繰り返す
	while(feof(fp)==0)
	{
			//ファイルからデータを読み込み、構造体配列に格納
			if((fscanf(fp,"%d,%[^,],%[^,],%[^,],%s",&(member[i].no),member[i].name,member[i].adress,member[i].tel,member[i].mail)) != EOF)
  			{
				//キーボードの入力値と構造体配列の番号が一致する時
				if(no==member[i].no)
				{
				   printf("\n%d",member[i].no); //会員番号
				   printf("\n%s",member[i].name); //名前
				   printf("\n%s",member[i].adress); //住所
				   printf("\n%s",member[i].tel); //電話番号
				   printf("\n%s",member[i].mail); //メールアドレス
				   printf("\n%s","削除しますか？（1:yes 2:no）？ ");
				   scanf("%d",&choice); //選択値を入力
				   
				   //削除しない時
				   if(choice==NO)
				   {
						system("cls"); //コマンドプロンプト画面クリア
						return CANCEL; //メイン関数に戻る
				   }
				}
			}
			i++;
	}
	i--;
	member[i]=member1; //構造体配列に終端値を代入
	i = 0;
	fclose(fp);	//ファイルを閉じる
	fp = fopen(fname, "w"); //書き込みモードでオープンしファイルをクリア
	fclose(fp); //ファイルを閉じる
	fp = fopen(fname, "w"); //ファイルを書き込みモードでオープン
	
	//会員番号がEOFでない場合
	while(member[i].no!=EOF)
	{
		//入力した会員番号と配列内の会員番号が等しくない時
		if(no!=member[i].no)
		{
			ret = csv_write(i);
		}
		i++;
	}
	fclose(fp);
	printf("%s","削除されました。");
	return NOERROR;
}

/*一覧表示*/
int disp_record()
{
	//読み込みモードでファイルをオープン
	fp = fopen(fname,"r");
	//ファイルポインタがNULLの場合
	if(fp == NULL)
	{
		printf("%sファイルが開けません\n",fname);
		return ERROR; //-1をエラー返り値として返す
	}
	
	//ファイル読み込み終了まで繰り返す
	while(feof(fp)==0)
	{
		//ファイルからデータを読み込み、構造体配列に格納
		if((fscanf(fp,"%d,%[^,],%[^,],%[^,],%s",&(member[i].no),member[i].name,member[i].adress,member[i].tel,member[i].mail)) != EOF)
  		{
			   printf("\n%d",member[i].no); //番号
			   printf("\n%s",member[i].name); //名前
			   printf("\n%s",member[i].adress); //住所
			   printf("\n%s",member[i].tel); //電話番号
			   printf("\n%s",member[i].mail); //メールアドレス
		}
	}
	printf("\n\n%s\n","メニューに戻ります。1:yes");
	scanf("%d",&choice); //メニューに戻るための入力
	//入力値が1の場合
	if(choice==1)
	{
		system("cls"); //コマンドプロンプトの表示クリア
		return CANCEL; //メイン関数に戻る
	}
	fclose(fp); //ファイルを閉じる
	return NOERROR;
}

int csv_read()
{
	fp = fopen(fname,"r"); //ファイルを読み込みモードで開く
	
	//ファイルポインタがNULLかどうか判定
	if(fp == NULL)
	{
		printf("%sファイルが開けません\n",fname);
		return ERROR; //エラーの返り値として-1を返す
	}
	return NOERROR;
}
int csv_write(int rno)
{
	fprintf(fp,"%d,%s,%s,%s,%s\n",member[rno].no,member[rno].name,member[rno].adress,member[rno].tel,member[rno].mail);
	return NOERROR;
}

int init_disp()
{
	int no = 0; //数字の入力値
		
	printf("---会員管理システムメニュー---\n\n");
	printf("1.新規登録\n");
	printf("2.変更\n");
	printf("3.削除\n");
	printf("4.一覧表示\n");
	printf("No? ");
	scanf("%d",&no); //数値を入力
	
	return no;
}

[h2so5]

Pnoをグローバル変数にする意味がないと思います。

[box]

コード:

#define MENU 0
#define ADDR 1

どういう意味を持っているか、コメントを書いてください。
コードを読む側は、何を意味しているのか全くわかりません。
「3ヶ月後の自分は他人」というプログラミング上の格言のとおり、
あとで修正する必要が生じたときなどのために、こういう箇所こそコメントを書いておくべきです。

コード:

FILE *fp; //ファイルポインタ
char *fname = "comma.csv"; //ファイル名

グローバル変数を使うことで、コードの可読性や保守性などが低下するおそれがあります。
使う際は、必要最小限にとどめた方がいいと思います。
また、グローバル変数の名前は、意味のあるものにしましょう（i などという名前は最悪）。

コード:

/*メイン関数*/
main()
{
Pno = 0;
Pno = init_disp();

インデントをきちんとしましょう。コードが読みづらくなります。

コード:

	fp = fopen(fname,"a"); //上書きモードでファイルを開く
	//ファイルポインタがNULLの時
	if(fp == NULL)

こういうコメントは不要だと思います。コードを見れば、だれでもわかります。
意味がある箇所に、意味があるコメントを書きましょう。

[ookami]

こんにちは。ookamiです。
気になったところを３つ書きますね。

> scanf("%s",member[0].name); //名前
バッファオーバーフローの危険性。

> fp = fopen(fname,"a"); //上書きモードでファイルを開く
「上書きモード」ではなく「追加モード」の方が混乱が無い気がする。

> printf("\n会員番号（0-10数値）？ ");
> scanf("%d",&no); //会員番号の入力
0-10以外の数値が入力された時のエラー処理が無い。

[nil]

コード:

#define MENU 0
#define ADDR 1
#define UPDATER 2
#define DELR 3
#define DISPR 4
#define CANCEL 5

列挙体にしたほうがわかりやすいかと。

コード:

fp = fopen(fname, "w"); //書き込みモードでオープンしファイルをクリア
fclose(fp); //ファイルを閉じる
fp = fopen(fname, "w"); //ファイルを書き込みモードでオープン

一回オープンしただけでクリアされませんでしたっけ？
違ってたらごめんなさい

[へにっくす]

私のコメント見てないようですのでリンクしときます。

http://dixq.net/forum/viewtopic.php?f=3&t=11000#p88579

[taz]

[quote="涼雅"]

コード:

#define MENU 0
#define ADDR 1
#define UPDATER 2
#define DELR 3
#define DISPR 4
#define CANCEL 5

列挙体にしたほうがわかりやすいかと。

列挙体で書こうとすると、うまくいきません。どのように書けばよいのでしょうか？

> scanf("%s",member[0].name); //名前
バッファオーバーフローの危険性。

バッファオーバーフロの危険性とはどういうことでしょうか？どうすればよいのでしょうか？

[box]

列挙体で書こうとすると、うまくいきません。どのように書けばよいのでしょうか？

どのようにうまくいかないかを書いていただくと、解決策が提示できるかもしれません。

バッファオーバーフロの危険性とはどういうことでしょうか？どうすればよいのでしょうか？

配列として定義している領域を超えた文字数の入力を行なったとき、今はノーチェックなので、
別の変数のために確保してある領域をこわしてしまうおそれあり、ということだろうと思います。
今のコードですでに使っておられるとおり、scanf()の書式文字列で ^ あたりを使って、
入力文字数に制限を加えるようにすればいいのではないか、と思います。

scanf()の書式文字列で"%d"、つまり整数値を受け取るつもりのところで
数字以外の文字を入力したらどういうことになるかわからない、という点についても
修正の余地がありそうな気がします。

[taz]

色々とアドバイスありがとうございました。またよろしくお願いします。