C:文字の判定関数を作る

Question

あるゲームでランキングに入賞したら名前を登録する関数を作ってみたのですが、上手く動作しません。
コンパイルしたときにエラーは吐きません。
この関数を作る条件に
①名前の入力にはfgets関数を用いる。(scanf関数は使わない。)
②ASCIIコードを使う。（環境依存は気にしなくてもよい。)(自分の環境にあわせたASCIIコードを使っている。)
③文字の判定に既存の関数は使わず、自力で判定する。（isalpha関数などは使わない。）
④使用可能文字は、(小文字、大文字、空白(' ')、コンマ('.'))

void input_name(char player_name[]){
    int i,j,flag;
    int  alpha[54];
    size_t length;

    //使用可能の文字をリストにいれる
    // a~z
    for (i = 97, j = 0; i <= 121; i++){
        alpha[j++] = i;
    }
    // A~Z
    for (i = 65; i <= 89; i++){
        alpha[j++] = i;
    }
    // .(コンマ)
    alpha[52] = 32;
    // " "(空白)
    alpha[53] = 46;

    //名前を入力し、使用可能か判定する
    while(1){
        flag = 0;
        if (fgets(player_name, 10, stdin) == NULL || player_name[0] == '\n'){
            puts("名前は１～１０文字で入力してください。");
            continue;
        }
        length = strlen(player_name);
        if (player_name[length - 1] == '\n')
            player_name[--length] = '\0';
        for (i = 0; i < length; i++){
            int useable;
            useable = 0;
            for (j = 0; j < 54; i++){ 
                if (player_name[i] == alpha[j])
                    useable = 1;
                if (!useable){
                    puts("英字、数字、空白、ピリオド以外の文字は使わないでください。");
                    flag = 1;
                    break;
                }         
            if (flag == 1)
                break;
            }
        }
        if (flag == 0)
            break;
    }
}

実行例①

名前を入力してください
名前は１～１０文字で入力してください。
aiueo
Segmentation fault

実行例②

名前を入力してください
名前は１～１０文字で入力してください。
"aiueo"
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
123 .
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。
英字、数字、空白、ピリオド以外の文字は使わないでください。

名前は１～１０文字で入力してください。

期待する実行例①

名前を入力してください
aiueo12 .

期待する実行例②

名前を入力してください
aiu@a
英字、数字、空白、ピリオド以外の文字は使わないでください。
名前を入力してください
aiueoaifusidfusiodfusiofaid
名前は１～１０文字で入力してください。
aiueo

問題点

①aiueoと入力したときに出力される、Segmentation faultと出力される
②名前を入力する前に、なぜか「名前は1~10文字で入力してください。」が出力される。

追記

typedef struct {
double score;
char name[256];
}player;

int main(void){
:
:
player players[RANKING_SIZE+1];
:
:
input_name(players[your_ranking].name);
:
:
今回作ったinput_name関数はmain関数内でこのような背景を持って呼び出されています。

Answer 1

「許された文字だけで構成された文字列であるかを判定する関数」を書いてみた

#include <stdio.h>

/* str 中に ch があれば 1 を、さもなくば 0 を返す */
int contains_chr(const char* str, char ch) {
  while ( *str != '\0' ) {
    if ( *str == ch ) return 1;
    ++str;
  }
  return 0;
}

/* target中にあるすべての文字がstr中にあるなら 1 さもなくば 0 を返す */
int contains_str(const char* str, const char* target) {
  while ( *target != '\0' ) {
    if ( !contains_chr(str, *target) ) return 0;
    ++target;
  }
  return 1;
}

/* おためし */
int main(void) {
  const char* table[] = { "aiueo", "aiue0", "abcABC .", NULL };
  const char* valid_chars = 
    "abcdefghijklmnopqrstuvwxyz"
    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    " .";
  int i;
  
  for ( i = 0; table[i] != NULL; ++i ) {
    printf("[%s] : %s\n", table[i], contains_str(valid_chars, table[i]) ? "OK" : "NG");
  }
  return 0;
}

/* 実行結果
[aiueo] : OK
[aiue0] : NG
[abcABC .] : OK
*/

「"単機能で確実に動く小さな関数"を積み上げて目的を達成する」クセをつけとくと幸せになれるよ♪

Comments

[cresc]

回答ありがとうございます。
細かい部品に分けるように関数化をして組み立てたほうが確かに読みやすいですね！
以前、別の質問をした際に、一度しか呼ばない関数を定義するのは無駄とご指摘をいただいたのですが、そんなことはないですか？

[LouiS0616]

すみません、その指摘をした人間です。
epistemeさんの関数は充分汎化されていますので、無駄にならないかと思います。いちおう『今後再利用が望まれる場合は除きます』と言及したのですが、誤解を招くような表現をしてすみません。
『今後再利用するか』よりも『今後再利用できるか』の方が表現としてよかったかもしれません。

[cresc]

LouiS0616さんの発言の評価を他の方に尋ねてしまって申し訳ありません。大変失礼なことをしたのではないかと反省しております。
たしかにepistemeさんの作った関数は汎用性があって今後再利用できますね。申し訳ありません、LouiS0616さんのおっしゃったことを間違えて受け取ってしまってました。関数化するときは単純で小さな機能にすることで汎用性が上がると思うので、これからはそれを意識してみます。

[episteme]

再利用されなくとも、テストが楽。このコードみたいにいくつかパターンを用意してちゃっちゃとおためしできる。

Answer 2

他の方が書かれていますが、こういう場合は、初期化と判定処理は分けて書いた方が、可読性やメンテナンス（デバッグ）が楽になります。それと、コード中に埋め込まれた数字（マジックナンバー）は第3者には意味不明になります。
「例」コンパイル時に-std=c99オプションを忘れないで

#define YES (0 == 0)
#define NO (!YES)

static int flag[256] = {NO};

void init()
{
    for (size_t i = '0'; i <= '9'; i++) {
        flag[i] = YES;
    }
    for (size_t i = 'A'; i <= 'Z'; i++) {
        flag[i] = YES;
    }
    for (size_t i = 'a'; i <= 'z'; i++) {
        flag[i] = YES;
    }
    flag[(size_t)' '] = YES;
    flag[(size_t)'.'] = YES;
}

int test(const char* name)
{
    if (name == NULL || *name == '\0' || *name == '\n') {
        return NO;
    }
    while (*name) {
        if (flag[(size_t)(*name)] == NO) {
            return NO;
        }
        name++;
    }
    return YES;
}

Comments

[cresc]

回答ありがとうございます。
皆さんのおっしゃる通り、関数に分けたほうが綺麗で読みやすいですね。
定数を直接コードに書くのは避けた方がいいみたいですね。勉強になります。
cateyeさんの定義しているYESは数値の1と定義されてますか？それともTrueみたいなものですか？

[cateye]

私が使う書き方ですが・・・アセンブラソースを見る限り、YESは1にNOは０に展開されているようです。(clang version 6.0.0)　fgets()を使った場合の行末'\n'の対処はしていませんが必要なら、init()にflag[(size_t)'\n'] = YES;を付け加えて見てください。悪さはしないと思います・・・たぶんd＾＾；

[raccy]

yesとnoだけであれば素直にstdbool.hを読み込んで`true`と`false`を使った方が良いかと思うのですが、使わずに`(0 == 0)`という感じにしている理由は何かあるのでしょうか？c90でも使うからですか？

[cateye]

特に意味はありません＾＾；　・・・OK,NGやON,OFFも使いますね・・・しいて言うなら、文脈しだいでしょうか？

[raccy]

いや、YESとNOの名前の話ではなくて、`true`を使わずに`(0 == 0)`と書いている意味を知りたかったのですが。

[cateye]

話がずれているかとも思いますが・・・、trueやfalseもstdbool.hで定義されているマクロですよね？　あえて使う必要があるのでしょうか？・・・いずれにしても単なるint型で真は１、偽は０になるわけですから・・・（true,falseを使うのがおかしいと言っているわけではありません。それこそ好みの問題だと思います）・・・
#include <stdio.h>
#include <stdbool.h>

#define YES (true)
#define NO (false)
・・・でも良い分けですから・・・

[raccy]

すいません。好みの問題と言われたらどうしようもないのですが、`1`とか`true`とか書かずに`0 == 0`と書いているのか疑問に思っただけです。特に理由も無く、cateyeさんがただの好みで使っているという話であれば、それで納得しました。

[cateye]

有り難うございますm(_"_)m　昔（FreeBSD３のころ）から使ってる記法なので、ついつい＾＾；

[Eki]

この例の場合、 (!YES) が 0 以外に展開されることはないので問題はないと思いますが、私なら NO を素直に 0 と定義します。(あるいは条件分岐で NO を使った判定をしないかも) 。
flag[256] = {NO}; という記述は flag の全要素を NO にするわけではなく、最初の要素を NO にして、後の要素をゼロクリアするという意味になるから、というのが主な理由です。何か書き間違えて NO が 0 以外の整数値になった場合、if (flag[(size_t)(*name)] == NO) {} の条件が (*name == 0を除いて) 成功してしまいますしね。

あっ、これはただのコメントなので、cateyeさんの書き方がどうだという話では全くありません。

Answer 3

アルゴリズムのおハナシが出てきたので、
勢いあまってC++版:

#include <iostream>
#include <algorithm>
#include <set>
#include <string>

/* target中にあるすべての文字がstr中にあるなら true さもなくば false を返す 
   ただし str は文字重複してはならず、sortされていること。*/
bool contains_str(const std::string& str, const std::string& target) {
  // targetに含まれる文字をそれぞれ一つ含み、sortされた文字集合tgtを作る。
  std::set<char> tgt(target.begin(), target.end());
  // strがtgtを包含するならtrue
  return std::includes(str.begin(), str.end(), tgt_.begin(), tgt_.end());
}

/* おためし */
int main(void) {
  using std::string;
  const string table[] = { "aiueo", "aiue0", "abcABC ." };
  string valid_chars = 
    "abcdefghijklmnopqrstuvwxyz"
    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    " .";
  std::sort(valid_chars.begin(), valid_chars.end());
  
  for ( const string& str : table ) {
    std::cout << str << " : " 
              << (contains_str(valid_chars, str) ? "OK" : "NG") << std::endl;
  }
}

/* 実行結果
[aiueo] : OK
[aiue0] : NG
[abcABC .] : OK
*/

Answer 4

新たな回答ではありませんが、質問者さん達には味わい深い問題だと思うので。

使える文字をalpha[]配列に格納しておき、player_name[]にある文字がalpha[]に含まれるかどうかで判定しよう・・・このアイディアは、epistemeさんが示されたcontains_str()と同じアルゴリズムだということに気づいてますか。

私の目から見て違って見えるのは、contains_str()は、int alpha[54]配列の代りに char *valid_chars = "abcd..." という文字配列を使って判定する事ですが、よく見れば、alpha[54]配列は valid_chars 文字配列を「書き写したもの」になっています。それなら valid_chars をそのまま使えば十分であって、alpha[]配列を使う旨味が感じられないことを、まず指摘しておきます。

contains_str()はcontains_chr()関数を呼びます。どちらの関数もwhileループがあるので、文字列一つの判定に二重のループを回す必要があるということなのです。

それに対して、cateyeさんが示したコードは判定の仕方が違うことに気づいてますか？判定部分を（ポインタではなく）配列を使い、噛み砕いて書き直すとこうなります。

    // 文字列中の各文字が使用可か否か、チェックする
    for (i = 0; i < length; i++) {
        int code = player_name[i];  // 一文字を取り出し
        if (flag[code] == NO) {     // そのアスキーコードは使用可か？
            puts("NG");
	// 以下省略

こちらは文字列一つの判定が一回のループで済むことに注意。二重ループよりも手間がかからないうえに、一文字の判定が一度の配列アクセスで済むという、とても効率の良い方法なのです。要は、アルゴリズムが違う。

同時にデータ構造も違う。int flag[256];という配列を使ってますね。この配列の意味、alpha[54]との違いを理解できてますか？
alpha[54]よりメモリを多く使うけど、最近のコンピュータはメモリをふんだんに積んでますから、効率の良いアルゴリズムのためにこそメモリを使うべきでしょう。こちらには配列を使う大きな旨味があります。

なお、flag[256]の内容を初期化する手間が発生しますが、初期化は一回で済むのに対し、判定は文字列の数だけ繰り返すのだから、判定の効率を上げたほうが良いと考えましょう。

質問者さん達には、このアルゴリズムの違いを味わっていただきたいものです。

Answer 5

他の方が指摘されているように、fgetsで入力を読み込むときは、長い文字列が入力された場合を考えて十分に長い領域を用意して下さい。中身の切り出しとチェックは、その後で行います。

char buf[1024];
fgets(buf, 1024, stdin);

Comments

[cresc]

回答ありがとうございます。
期待する文字列よりも長い文字列が代入されたら不具合が起きるみたいですね。
これからは気を付けます。
ご指摘ありがとうございました。

Answer 6

とりあえず、

            for (j = 0; j < 54; i++){ 

ではなく

            for (j = 0; j < 54; j++){ 

にするべきでしょうか。
あと、

            for (j = 0; j < 54; i++){ 
                if (player_name[i] == alpha[j])
                    useable = 1;
                if (!useable){
                    puts("英字、数字、空白、ピリオド以外の文字は使わないでください。");
                    flag = 1;
                    break;
                }         
            if (flag == 1)
                break;
            }

```よりは
```C
            for (j = 0; j < 54; j++){ 
                if (player_name[i] == alpha[j]) {
                    useable = 1;
                    break;
                }
            }
            if (!useable){
                puts("英字、数字、空白、ピリオド以外の文字は使わないでください。");
                flag = 1;
                break;
            }

```のほうが正しいのかな、という気がします。(動作未確認。)
LouiS0616さんの回答にもあるように、適切に関数化したほうが読みやすくできそうですが。

質問の内容には含まれませんが、書いておきます。
・fgets(player_name, 10, stdin) では10文字分の入力を得られないはずですよ
・長い文字列が入力された場合、fgetsで余った入力分は次のfgets（あるいは別の入力処理）に回されますよ
・自分の環境限定のプログラムでも、65よりも'A'で書いたほうがわかりやすいと思いますよ
・数字を受け付ける処理になってないけど、putsの文章ではそうは読めませんよ
・デバッガの使い方を身に付けたら便利ですよ
・個人的には、ifの条件が成立したときの実行文は1行でも{}で囲む。あるいはifと同じ行に書く。

Comments

[cresc]

回答ありがとうございます。
ミスのご指摘、たくさんのアドバイスありがとうございます。おっしゃる通り間違えてました。
数字の判定もうっかり忘れてしまってました。
たしかに関数化した方が読みやすくなりそうですね。epistemeさんのやり方で関数化してみます。
たくさんいただいたアドバイスを参考にさせていただきます。
本当にありがとうございました。

Answer 7

input_nameをどのように呼び出しているのかわからないので、憶測の多い回答です。
main関数などについても、追記していただけると助かります。

Segmentation Fault

input_nameの引数の与え方が問題かと思います。
次のように書いていませんか？

char *name;
input_name(name);

配列として宣言するか、mallocして領域を確保する必要があります。

名前は１～１０文字で入力してください。

バッファにゴミが残っているのだと予想します。
input_nameに入る前に、何か標準入力を処理していませんか？

英字、数字、空白、ピリオド以外の文字は使わないでください。

ご提示のコードだと、alpha[0]と合致しない限り通らないかと思います。

for (j = 0; j < 54; i++){
if (player_name[i] == alpha[j])
useable = 1;
if (!useable){
puts("英字、数字、空白、ピリオド以外の文字は使わないでください。");
flag = 1;
break;
}

可読性を考えても、判定する関数を別に用意してはいかがですか。

Comments

[cresc]

回答ありがとうございます。input_name関数が呼ばれているmain関数の部分を追記いたしましたので、ご確認ください。

[cresc]

引数に与えているものは、リスト構造の構造体のchar型です。

[cresc]

input_name関数の前にscanf()での入力を受け付けてます。バッファの使い方がscanf()とfgets()では違うからfgets()の前に使ったらダメみたいですね。この場合、fgets()の直前に、fpurge()でバッファを綺麗にすることで解決できますか？

[cresc]

ご指摘の通り、この場合だとalpha[0]と合致する場合しか通らない書き方になってますね。
okrtさんに教えていただいたやり方に書き直しました。

[LouiS0616]

セグフォですが、これはokrtさんが指摘しているインクリメントの間違いに起因していますね。
引数の与え方は問題ないように思います。

[LouiS0616]

実はfpurgeという関数を初めて聞いて慌てて調べたんですが、やはりCの標準関数ではないみたいですね。
おそらく改行文字が残ってしまっているので、getcharで空読みしてやればいいかと思います。

[cresc]

私は大学1回生で、教授がscanf()とfgets()を一緒に使ってはいけない。応急処置としてfpurge()を使えばいいが、これは偽薬ぽいやり方だ。といってたのを思い出しました。
おかげさまで無事に解決することができました。丁寧な回答ありがとうございました。

Answer 8

「文字列から或る文字を検索する」というのは標準ライブラリ関数strchr()の機能そのものです。
これを用いると、epistemeさんが作成されたcontains_chr()は

#include <string.h>

/* str 中に ch があれば 1 を、さもなくば 0 を返す */
int contains_chr(const char* str, char ch) {
  return strchr(str, ch) != NULL;
}

と簡潔に書けます。