回答編集履歴

3 不要な文字が残っていたので削除。

rubato6809

rubato6809 score 1133

2017/07/11 00:02  投稿

コーディングスタイルは私と違いますが、コードに律儀な性格が表れているように感じられ、好ましく思いました。同時に経験不足なども感じました。
私は、先輩のコードを読み解いてリストの操作を理解しました。理解しましたが、いざ書こうとすると、リスト構造の絵を描いて、これをここに挿入するには、このポインタを、こっちに代入して・・・と、いちいち絵を描いています(苦笑)。そういうものだ、という事を含めて、参考にしていただけると幸いです。
気づいた事をいくつか挙げます。
NULL と書くべき所に '\0' を使っています。数値としてはどちらも0ですが、**'\0' はヌル文字の値**であり、文字変数に対して使うべき。一方、**NULLはヌルポインタの値**です。ポインタに対する比較・代入はNULLを使いましょう。
read_word()が返す値 0, 1, n の意味は、
0 -- 既にEOFに到達した
1 -- buf[] に格納しない文字(',', '.', '\n', ' ')が続いた場合
n -- 入力した文字列の長さ|列1|列2|列3|
n -- 入力した文字列の長さ
かと思いました。文字列長が1の場合("I", "a")があるので、おそらくそれを考慮して
```C
   a = read_word(fp, buf);
   ...
   if(a == 1){
         if(w->name[0] != '\0'){
```
としたのでしょうが、余計に複雑にしています。私なら、
 -1 -- 既にEOFに到達した(普通EOFは -1 に定義されている)
 0 -- buf[] に格納しない文字(',', '.', '\n', ' ')
 1以上 -- 単語の文字列長
とします。EOFをそのまま返せますし、(0の場合も含めて)文字列長をそのまま返せるので、コードがシンプルになります。
フラグとして使っているend_of_file変数は、値として0と1を代入していますが、0と1でさえもマジックナンバになる事があります。<stdbool.h> があれば、bool型が定義済みですし、trueとfalseを値として使えます。その方が意味が伝わりやすくなります。
そのend_of_file変数をグローバル変数にしていますが、必要なのは read_word() 関数だけですから、関数内のstatic変数にできるし、したほうが良いです。
fgetc(), getchar()等で、一文字を読み、かつ、EOFの判定をするなら、char型変数で受け取るのではなく、int型で受け取ってください。
if〜else〜の中でreturn/break/continueする場合、組み方を変えればインデントを浅くでき、可読性が上がります。
構造体の型名はtypedefしてしまえば、いちいちstructと書かずに済みます(笑)。新しく作った型名は、マクロ名と同様、大文字で書く(少なくとも先頭は大文字にする)ことで、変数名等との混同を避けることができます。
さて、ソートを考えた時、線形リストというデータ構造はあまり得策ではないと思います。こんなページを発見しました。
[連結リストのソート](http://d.hatena.ne.jp/yarb/20090512/p1)
「ややこしくて途中で挫折した」他にコメントもあまり芳しくありません。ですが、こんな手順が浮かびました。
リストに10個データがあるとします。リストを最後まで辿れば、10個の中からリストの先頭に来るべき要素が見つかります。その要素をリストから削除(抜き出)し、リストの先頭に挿入し、これで先頭が決まります。
その次は、残り9個の要素に対して、同様の処理を行い、これで2番目が決まります。
その次は、残り8個(以下省略)、という具合い。
下のコードをご覧ください。手元で簡単なテストはパスしました。
要素を削除する・要素を挿入する、はリスト構造の得意とするところ…とは言え、上述の通り、絵を描かないと処理を間違えますし、変数がいくつ必要なのかさえ迷います(苦笑)。
もうひとつ、head変数をポインタにするのは、一見素直なようでいて、手順が複雑になる要因だと思いました。ソートの手順と合わせて考えた結果、ポインタではなく、空の構造体変数にしてみました(これは人によってやり方が違うかもしれない)。実際に使っているのは head.next ポインタだけです。
```C
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>   // or "typedef enum { false, true } bool;"
typedef struct word {
   struct word *next;
   int count;
   char name[20];
} WORD;
WORD head = {         // ポインタではなく、空の構造体にしてみた
   NULL, 0, { '\0' }
};
WORD *new(char *buf)
{
   WORD *p;
   p = malloc(sizeof(WORD)); // キャストは不要
   strcpy(p->name, buf);     // 文字列を格納、構造体を初期化
   p->next = NULL;
   p->count = 1;
   return p;
}
WORD *search(WORD *p, char *buffer)
{
   WORD *temp = p;
   while (temp) {
       if (strcmp(temp->name, buffer) == 0)
           return temp;
       temp = temp->next;
   }
   return NULL;
}
int read_word(FILE *file, char *buffer)
{
   static bool end_of_file = false;
   int len = 0;   // 文字列長を返す
   if (end_of_file == true)
       return EOF;
   while (1) {
       int ch = fgetc(file);
       switch (ch) {
       case EOF:
           end_of_file = true;
           /* NO_BREAK */
       case ',':
       case '.':
       case '\n':
       case ' ':
           buffer[len] = '\0'; // 文字列終端は、ここだけ
           return len;
       default:
           buffer[len++] = ch; // 文字を格納
           break;
       }
   }
}
void delete(void)
{
   WORD *curr, *temp;
   for (curr = head.next; curr != NULL; curr = temp) {
       temp = curr->next;
       free(curr);
   }
}
void reading(void)
{
   WORD *tail, *b;
   char buf[20];
   int wlen;       // 文字列長
   FILE *fp;
   if ((fp = fopen("anne.txt", "r")) == NULL) {
       printf("failed to open file\n");
       exit(1);
   }
   tail = &head;    // 最初はリスト先頭が最後尾
   while (1) {
       wlen = read_word(fp, buf);
       if (wlen == EOF) {
           return;
       }
       if (wlen == 0) {
           continue;
       }
       b = search(head.next, buf);
       if (b == NULL) {
           // 初出現なので新規登録
           WORD *t = new(buf); // new()の呼出はここだけ
           tail->next = t;      // 最後尾につなぐ
           tail = t;
       } else {
           b->count++;         // 出現回数を+1
       }
   }
   fclose(fp);
}
void sorting(void)
{
   WORD *top, *max, *prev, *curr, *succ;
   for (top = &head; top->next != NULL; top = top->next) {
       /* top->next から終わりまで「最大」エントリを探す */
       max = curr = top->next; // とりあえず先頭を最大とする
       prev = top;            // 最大の一つ前を覚えておく
       while ((succ = curr->next) != NULL) {
           if (strcmp(succ->name, max->name) < 0) { // 比較
               max = succ;    // 最大エントリを更新
               prev = curr;   // 一つ前も更新
           }
           curr = curr->next; // 一歩前へ
       }
       /* 最大エントリmaxが見つかった。交換が必要か? */
       if (max != top->next) {
           prev->next = max->next;  // リストからmaxを抜き出し
           max->next = top->next;   // maxをtopの次に挿入する
           top->next = max;
       }
   }
}
void showList(void)
{
   WORD *w;
   for (w = head.next; w != NULL; w = w->next)
       printf("%4d %s\n", w->count, w->name);
}
int main(void)
{
   reading();   showList();
   printf("--- sorting ---\n");
   sorting();   showList();
   delete();
   return 0;
}
```
2 誤字をなおした。

rubato6809

rubato6809 score 1133

2017/07/10 22:43  投稿

コーディングスタイルは私と違いますが、コードに律儀な性格が表れているように感じられ、好ましく思いました。同時に経験不足なども感じました。
私は、先輩のコードを読み解いてリストの操作を理解しました。理解しましたが、いざ書こうとすると、リスト構造の絵を描いて、これをここに挿入するには、このポインタを、こっちに代入して・・・と、いちいち絵を描いています(苦笑)。そういうものだ、という事を含めて、参考にしていただけると幸いです。
気づいた事をいくつか挙げます。
NULL と書くべき所に '\0' を使っています。数値としてはどちらも0ですが、**'\0' はヌル文字の値**であり、文字変数に対して使うべき。
一方、**NULLはヌルポインタの値**です。ポインタに対する比較・代入はNULLを使いましょう。
NULL と書くべき所に '\0' を使っています。数値としてはどちらも0ですが、**'\0' はヌル文字の値**であり、文字変数に対して使うべき。一方、**NULLはヌルポインタの値**です。ポインタに対する比較・代入はNULLを使いましょう。
read_word()が返す値 0, 1, n の意味は、
0 -- 既にEOFに到達した
1 -- buf[] に格納しない文字(',', '.', '\n', ' ')が続いた場合
n -- 入力した文字列の長さ|列1|列2|列3|
かと思いました。文字列長が1の場合("I", "a")があるので、おそらくそれを考慮して
```C
   a = read_word(fp, buf);
   ...
   if(a == 1){
         if(w->name[0] != '\0'){
```
としたのでしょうが、余計に複雑にしています。私なら、
 -1 -- 既にEOFに到達した(普通EOFは -1 に定義されている)
 0 -- buf[] に格納しない文字(',', '.', '\n', ' ')
 1以上 -- 単語の文字列長
とします。EOFをそのまま返せますし、(0の場合も含めて)文字列長をそのまま返せるので、コードがシンプルになります。
フラグとして使っているend_of_file変数は、値として0と1を代入していますが、0と1でさえもマジックナンバになる事があります。<stdbool.h> があれば、bool型が定義済みですし、trueとfalseを値として使えます。その方が意味が伝わりやすくなります。
そのend_of_file変数をグローバル変数にしていますが、必要なのは read_word() 関数だけですから、関数内のstatic変数にできるし、したほうが良いです。
fgetc(), getchar()等で、一文字を読み、かつ、EOFの判定をするなら、char型変数で受け取るのではなく、int型で受け取ってください。
if〜else〜の中でreturn/break/continueする場合、組み方を変えればインデントを浅くでき、可読性が上がります。
さて、ソートを考えた時、線形リストというデータ構造はあまり得策ではないと思います。こんなページを発見しました。
[連結リストのソート](http://d.hatena.ne.jp/yarb/20090512/p1)
「ややこしくて途中で挫折した」他にコメントもあまり芳しくありません。ですが、こんな手順が浮かびました。
リストに10個データがあるとします。リストを最後まで辿れば、10個の中からリストの先頭に来るべき要素が見つかります。その要素をリストから削除(抜き出)し、リストの先頭に挿入し、これで先頭が決まります。
その次は、残り9個の要素に対して、同様の処理を行い、これで2番目が決まります。
その次は、残り8個(以下省略)、という具合い。
リストをご覧ください。手元で簡単なテストはパスしました。
下のコードをご覧ください。手元で簡単なテストはパスしました。
要素を削除する・要素を挿入する、はリスト構造の得意とするところ…とは言え、上述の通り、絵を描かないと処理を間違えますし、変数がいくつ必要なのかさえ迷います(苦笑)。
もうひとつ、head変数をポインタにするのは、一見素直なようでいて、手順が複雑になる要因だと思いました。ソートの手順と合わせて考えた結果、ポインタではなく、空の構造体変数にしてみました(これは人によってやり方が違うかもしれない)。実際に使っているのは head.next ポインタだけです。
```C
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>   // or "typedef enum { false, true } bool;"
typedef struct word {
   struct word *next;
   int count;
   char name[20];
} WORD;
WORD head = {         // ポインタではなく、空の構造体にしてみた
   NULL, 0, { '\0' }
};
WORD *new(char *buf)
{
   WORD *p;
   p = malloc(sizeof(WORD)); // キャストは不要
   strcpy(p->name, buf);     // 文字列を格納、構造体を初期化
   p->next = NULL;
   p->count = 1;
   return p;
}
WORD *search(WORD *p, char *buffer)
{
   WORD *temp = p;
   while (temp) {
       if (strcmp(temp->name, buffer) == 0)
           return temp;
       temp = temp->next;
   }
   return NULL;
}
int read_word(FILE *file, char *buffer)
{
   static bool end_of_file = false;
   int len = 0;   // 文字列長を返す
   if (end_of_file == true)
       return EOF;
   while (1) {
       int ch = fgetc(file);
       switch (ch) {
       case EOF:
           end_of_file = true;
           /* NO_BREAK */
       case ',':
       case '.':
       case '\n':
       case ' ':
           buffer[len] = '\0'; // 文字列終端は、ここだけ
           return len;
       default:
           buffer[len++] = ch; // 文字を格納
           break;
       }
   }
}
void delete(void)
{
   WORD *curr, *temp;
   for (curr = head.next; curr != NULL; curr = temp) {
       temp = curr->next;
       free(curr);
   }
}
void reading(void)
{
   WORD *tail, *b;
   char buf[20];
   int wlen;       // 文字列長
   FILE *fp;
   if ((fp = fopen("anne.txt", "r")) == NULL) {
       printf("failed to open file\n");
       exit(1);
   }
   tail = &head;    // 最初はリスト先頭が最後尾
   while (1) {
       wlen = read_word(fp, buf);
       if (wlen == EOF) {
           return;
       }
       if (wlen == 0) {
           continue;
       }
       b = search(head.next, buf);
       if (b == NULL) {
           // 初出現なので新規登録
           WORD *t = new(buf); // new()の呼出はここだけ
           tail->next = t;      // 最後尾につなぐ
           tail = t;
       } else {
           b->count++;         // 出現回数を+1
       }
   }
   fclose(fp);
}
void sorting(void)
{
   WORD *top, *max, *prev, *curr, *succ;
   for (top = &head; top->next != NULL; top = top->next) {
       /* top->next から終わりまで「最大」エントリを探す */
       max = curr = top->next; // とりあえず先頭を最大とする
       prev = top;            // 最大の一つ前を覚えておく
       while ((succ = curr->next) != NULL) {
           if (strcmp(succ->name, max->name) < 0) { // 比較
               max = succ;    // 最大エントリを更新
               prev = curr;   // 一つ前も更新
           }
           curr = curr->next; // 一歩前へ
       }
       /* 最大エントリmaxが見つかった。交換が必要か? */
       if (max != top->next) {
           prev->next = max->next;  // リストからmaxを抜き出し
           max->next = top->next;   // maxをtopの次に挿入する
           top->next = max;
       }
   }
}
void showList(void)
{
   WORD *w;
   for (w = head.next; w != NULL; w = w->next)
       printf("%4d %s\n", w->count, w->name);
}
int main(void)
{
   reading();   showList();
   printf("--- sorting ---\n");
   sorting();   showList();
   delete();
   return 0;
}
```
1 脱字があったので。

rubato6809

rubato6809 score 1133

2017/07/10 21:56  投稿

コーディングスタイルは私と違いますが、コードに律儀な性格が表れているように感じられ、好ましく思いました。同時に経験不足なども感じました。
私は、先輩のコードを読み解いてリストの操作を理解しました。理解しましたが、いざ書こうとすると、リスト構造の絵を描いて、これをここに挿入するには、このポインタを、こっちに代入して・・・と、いちいち絵を描いています(苦笑)。そういうものだ、という事を含めて、参考にしていただけると幸いです。
気づいた事をいくつか挙げます。
NULL と書くべき所に '\0' を使っています。数値としてはどちらも0ですが、**'\0' はヌル文字の値**であり、文字変数に対して使うべき。
一方、**NULLはヌルポインタの値**です。ポインタに対する比較・代入はNULLを使いましょう。
read_word()が返す値 0, 1, n の意味は、
0 -- 既にEOFに到達した
1 -- buf[] に格納しない文字(',', '.', '\n', ' ')が続いた場合
n -- 入力した文字列の長さ|列1|列2|列3|
かと思いました。文字列長が1の場合("I", "a")があるので、おそらくそれを考慮して
```C
   a = read_word(fp, buf);
   ...
   if(a == 1){
         if(w->name[0] != '\0'){
```
としたのでしょうが、余計に複雑にしています。私なら、
 -1 -- 既にEOFに到達した(普通EOFは -1 に定義されている)
 0 -- buf[] に格納しない文字(',', '.', '\n', ' ')
 1以上 -- 単語の文字列長
とします。EOFをそのまま返せますし、(0の場合も含めて)文字列長をそのまま返せるので、コードがシンプルになります。
フラグとして使っているend_of_file変数は、値として0と1を代入していますが、0と1でさえもマジックナンバになる事があります。<stdbool.h> があれば、bool型が定義済みですし、trueとfalseを値として使えます。その方が意味が伝わりやすくなります。
そのend_of_file変数をグローバル変数にしていますが、必要なのは read_word() 関数だけですから、関数内のstatic変数にできるし、したほうが良いです。
fgetc(), getchar()等で、一文字を読み、かつ、EOFの判定をするなら、char型変数で受け取るのではなく、int型で受け取ってください。
if〜else〜の中でreturn/break/continueする場合、組み方を変えればインデントを浅くでき、可読性が上がります。
構造体の型名はtypedefしてしまえば、いちいちstructと書かずに済みます(笑)。新しく作った型名は、マクロ名と同様、大文字で書く(少なくとも先頭は大文字もする)ことで、変数名等との混同を避けることができます。
さて、ソートを考えた時、線形リストというデータ構造はあまり得策ではないと思います。こんなページを発見しました。
[連結リストのソート](http://d.hatena.ne.jp/yarb/20090512/p1)
「ややこしくて途中で挫折した」他にコメントもあまり芳しくありません。ですが、こんな手順が浮かびました。
リストに10個データがあるとします。リストを最後まで辿れば、10個の中からリストの先頭に来るべき要素が見つかります。その要素をリストから削除(抜き出)し、リストの先頭に挿入し、これで先頭が決まります。
その次は、残り9個の要素に対して、同様の処理を行い、これで2番目が決まります。
その次は、残り8個(以下省略)、という具合い。
リストをご覧ください。手元で簡単なテストはパスしました。
もうひとつ、head変数をポインタにするのは、一見素直なようでいて、手順が複雑になる要因だと思いました。ソートの手順と合わせて考えた結果、ポインタではなく、空の構造体変数にしてみました(これは人によってやり方が違うかもしれない)。実際に使っているのは head.next ポインタだけです。
```C
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>   // or "typedef enum { false, true } bool;"
typedef struct word {
   struct word *next;
   int count;
   char name[20];
} WORD;
WORD head = {         // ポインタではなく、空の構造体にしてみた
   NULL, 0, { '\0' }
};
WORD *new(char *buf)
{
   WORD *p;
   p = malloc(sizeof(WORD)); // キャストは不要
   strcpy(p->name, buf);     // 文字列を格納、構造体を初期化
   p->next = NULL;
   p->count = 1;
   return p;
}
WORD *search(WORD *p, char *buffer)
{
   WORD *temp = p;
   while (temp) {
       if (strcmp(temp->name, buffer) == 0)
           return temp;
       temp = temp->next;
   }
   return NULL;
}
int read_word(FILE *file, char *buffer)
{
   static bool end_of_file = false;
   int len = 0;   // 文字列長を返す
   if (end_of_file == true)
       return EOF;
   while (1) {
       int ch = fgetc(file);
       switch (ch) {
       case EOF:
           end_of_file = true;
           /* NO_BREAK */
       case ',':
       case '.':
       case '\n':
       case ' ':
           buffer[len] = '\0'; // 文字列終端は、ここだけ
           return len;
       default:
           buffer[len++] = ch; // 文字を格納
           break;
       }
   }
}
void delete(void)
{
   WORD *curr, *temp;
   for (curr = head.next; curr != NULL; curr = temp) {
       temp = curr->next;
       free(curr);
   }
}
void reading(void)
{
   WORD *tail, *b;
   char buf[20];
   int wlen;       // 文字列長
   FILE *fp;
   if ((fp = fopen("anne.txt", "r")) == NULL) {
       printf("failed to open file\n");
       exit(1);
   }
   tail = &head;    // 最初はリスト先頭が最後尾
   while (1) {
       wlen = read_word(fp, buf);
       if (wlen == EOF) {
           return;
       }
       if (wlen == 0) {
           continue;
       }
       b = search(head.next, buf);
       if (b == NULL) {
           // 初出現なので新規登録
           WORD *t = new(buf); // new()の呼出はここだけ
           tail->next = t;      // 最後尾につなぐ
           tail = t;
       } else {
           b->count++;         // 出現回数を+1
       }
   }
   fclose(fp);
}
void sorting(void)
{
   WORD *top, *max, *prev, *curr, *succ;
   for (top = &head; top->next != NULL; top = top->next) {
       /* top->next から終わりまで「最大」エントリを探す */
       max = curr = top->next; // とりあえず先頭を最大とする
       prev = top;            // 最大の一つ前を覚えておく
       while ((succ = curr->next) != NULL) {
           if (strcmp(succ->name, max->name) < 0) { // 比較
               max = succ;    // 最大エントリを更新
               prev = curr;   // 一つ前も更新
           }
           curr = curr->next; // 一歩前へ
       }
       /* 最大エントリmaxが見つかった。交換が必要か? */
       if (max != top->next) {
           prev->next = max->next;  // リストからmaxを抜き出し
           max->next = top->next;   // maxをtopの次に挿入する
           top->next = max;
       }
   }
}
void showList(void)
{
   WORD *w;
   for (w = head.next; w != NULL; w = w->next)
       printf("%4d %s\n", w->count, w->name);
}
int main(void)
{
   reading();   showList();
   printf("--- sorting ---\n");
   sorting();   showList();
   delete();
   return 0;
}
```

思考するエンジニアのためのQ&Aサイト「teratail」について詳しく知る