単純でわかりやすいプログラムとは・・・

[戻る]
神谷 昇 2001/11/01(木) 16:57:39
はじめまして。神谷と申します.
すっきりとして、第一印象で全体像が把握できるプログラムを作りたいと思っています。
自分ではこれ以上ないと思うものを作ってみたんですけど、よかったら足りない点を
指摘してもらえますか?一応、このままでも実行できます。特に//名前の順で並べるの部分
が自信がありません。私自身、経験が浅くて、インデントなど細かい部分で見苦しいかもし
れませんが、そういった部分のご指摘をいただけるとうれしいです。

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

//構造体の宣言
typedef struct SOURIdata
{
    char name[20];
    int toshi;
    int siji;
}SOURI;                    //構造体名

//ファイルの読み込み
void loadfile (SOURI* souri ,char *filename)
{
    FILE* fp;
    char buf[100];    
    char temp[10];
    char *p;
    int  n;

    fp = fopen(filename,"r");

    while(fgets(buf,100,fp)!=NULL){
        p = buf;

        //##### 名前の入力 #####
        n=0;
        while(*p != ','){            
            souri->name[n] = *p;        //カンマまでをコピー
            n++;
            p++;
        }
        souri->name[n] = '\0';
        p++;
    
        //##### 年齢の入力 #####
        n=0;
        while(*p != ','){       
            temp[n] = *p;
            n++;
            p++;
        }
        temp[n] = '\0';
        p++;
        souri->toshi = atoi(temp);

        //##### 支持率の入力 #####
        n=0;
        while(*p != '\n'){    
            temp[n] = *p;
            n++;
            p++;
        }
        temp[n] = '\0';
        souri->siji = atoi(temp);
    
        souri++;

    }

    souri->siji = 0;    //データの最後を示す

    fclose(fp);    
}
void drawdata (SOURI* souri)                               //SOURIを表示する。
{
    while( souri->siji != 0){
        printf("[名前]%10s [年齢]%2d歳 [最高指示率]%3d%%\n",
                    souri->name,souri->toshi,souri->siji);
        souri++;        
    }

int dataCount(SOURI *souri)        //登録データ数を返す
{
    int count = 0;
    while(souri->siji != 0){
        count++;
        souri++;        
    }
    return count;
}

//名前順に並べる
void sort1 (SOURI *souri)                                 
{
    int i;        
    int j;
    int n;
    int wk;
    SOURI nametop;
    n=dataCount(souri);
    
    wk=0;
    for(i=0;i<n-1;i++){
       for(j=i+1;j<n;j++){
         //#### 初めの文字の比較 ####
          if((souri+i)->name[wk]>(souri+(j))->name[wk]){            
              nametop=*(souri+(j));
              *(souri+(j))=* (souri+i);
              *(souri+i)=nametop;
        }
        //#### 初めの文字が同じ場合 ####
          else if((souri+i)->name[0]==(souri+(j))->name[0]){    
             while((souri+i)->name[wk]!=NULL || (souri+(j))->name[wk]!=NULL){
                 if((souri+i)->name[wk]>(souri+(j))->name[wk]){    
                     nametop=* (souri+(j));
                     * (souri+(j))=* (souri+i);
                     * (souri+i)=nametop;
                 }
                 wk++;    //nameの配列を増やす
             }
          }
          wk=0;    //wkの初期化
       }
    }
}

void main(void)

{
    SOURI sr[10];

    loadfile(sr,"c:\\workfile\\SOURI.csv");

    printf("ファイルには以下のデータが入っています。\n");
    drawdata(sr);

    printf("名前順で表示すると・・・・\n");
    sort1(sr);
    drawdata(sr);
}

NGA 2001/11/01(木) 18:57:47
私が気になった点は

1.定数
 #define BUF_SIZE    100
 としておいて、
 char buf[100]; → char buf[BUF_SIZE];
 とする。

2.ポインタ引数のNULLチェック
 引数である SOURI* souri ,char *filename のNULLチェックがない。

3.変数・仮引数に対するコメントがない。

4.ローカル変数の初期化がない。

5.FILE* fp のNULLチェックがない。

6.オペランド 演算子 オペランドでスペースがあったりなかったりして統一性がない。
 私的にはあけます。

7.変数の宣言の後、1行あけて処理を始める。
 できているところとできていないところがある。

8.void main(void)
 int main( int argc, char *argv[])
 もしくは
 int main( int argc, char *argv[], char *envp[])
 とする。

9.変数のネーミング(規則)

10.変数宣言
 データ型と変数名の間のスペースの数の規則性。

11.void drawdata (SOURI* souri)
 閉じられていない。

12.void main(void)
 最後の '}' が 全角です。

こんな感じです。
アルゴリズム云々は見ていません。

NGA 2001/11/01(木) 19:08:22
全体像を把握したいというなら、短いソースですが関数をプロトタイプ宣言しておくほうがいいかもしれませんね。

kazuma 2001/11/02(金) 01:37:51
すっきりさせるということなら、
loadfile 関数の中の「名前の入力」「年齢の入力」「支持率の入力」
によく似た部分があるのが気になりませんか?
関数として切り出すとすっきりします。

strtok とか qsort という関数が標準で用意されているので知っておくといいです。

読み込むファイルが10行以上あったらどうなるか、とか、
ファイルが期待した形式でなかったら、とか。この辺はプログラムの用途や方針にもよりますが。

sort1関数内の文字列の比較と思われる部分がとってもあやしいです。
文字列の比較は strcmp で出来ます。

細かいところでは、
void drawdata (const SOURI* souri) のほうが好きだったり。
支持率0でデータの終端を示すのがいいのかどうか、と思ったり。
*(souri+i) を souri[i] と書いてもいいのは知ってますよね?
文字列の終端文字'\0'を表すのにNULLを使うべきではないです。
int main(void) は可能です。void main(void) はいけませんが。

神谷 昇 2001/11/02(金) 08:52:33
[[解決]]
NGAさん、kazumaさん、どうもありがとうございます。
おふたりのコメントをもとに調べるべきところと、
純粋に、「なるほど」と理解できたところがありました。

掲示板を読むのが楽しかったです。

古いスレッドには追加発言できません。