「苦しんで覚えるc言語」のとあるコードをもっと安全に、より良く

はじめに

2016年01月22日にQiitaに
C言語で安全に標準入力から数値を取得
を投稿してからもう3年が経とうとしている。

初投稿からいくつも不具合が見つかり、つい先日もfgetsのエラーハンドリング周りでバグを見つけて改定した。C言語で安全に標準入力から数値を取得するのが難しすぎる。

QiitaやteratailでCの標準入力から数値を取得しているものを見るたびに上の記事へ誘導し続けてきた。

ところで先日
C++ - 自分の解答の穴を見つけて頂けると助かります。｜teratail
という質問を発見した。元のコードは「苦しんで覚えるc言語」のとあるコードらしいが、改善点がいくつもある。

さぁ、そろそろ「苦しんで覚えるc言語」に挑むか。

・・・ちなみに筆者は「苦しんで覚えるc言語」立ち見勢ですが、良書だと思ってます。独習Cなんざ投げ捨てるべし

元のコード

#include<stdio.h>
#include<string.h>
typedef struct
{
    char name[256];
    int age;
    int sex;
}People;

void InputPeople(People *data);
void ShowPeople(People data);

int main(void)
{
    People data[3];
    int i;

    for (i = 0; i < 3; i++)
    {
        InputPeople(&data[i]);
    }
    for ( i = 0; i < 3; i++)
    {
        ShowPeople(data[i]);
    }
    return 0;
}


void InputPeople(People *data)
{
    printf("名前：");
    scanf_s("%s", &data->name,256);
    printf("年齢：");
    scanf_s("%d", &data->age);
    printf("性別(1-男性、2-女性):");
    scanf("%d", &data->sex);
    printf("\n");
}


void ShowPeople(People data)
{
    char sex[16];

    printf("名前：%s\n",data.name);
    printf("年齢：%d\n",data.age);


    if (data.sex == 1)
    {
        strcpy_s(sex, 16,"男性");
    }
    else
    {
        strcpy_s(sex, 16,"女性");
    }
    printf("性別：%s\n",sex);
    printf("\n");
}

改善できそうなところ

MSVCに依存したコード

一部scanf_sじゃなくてscanfが見えるから、元のコードじゃなくて質問者が書き換えた疑惑があるけどそれはさておき。

_s付き関数は一部はC11で標準にoptionalで入ったが、常に使えるわけではないしなんかやっぱりCから取り除くみたいな話も聞く。使わないように書き換えるべきだろう。

scanf系関数で数値を読み込まない

C言語で安全に標準入力から数値を取得
でも他所でもさんざん指摘されているとおり、scanf系関数で安全に数値に変換することはできない。

構造体コピーの排除

コード中の構造体を見てみる。

typedef struct
{
    char name[256];
    int age;
    int sex;
}People;

256要素あるchar配列をメンバーに持つ。結構でかい構造体だ。しかし

void ShowPeople(People data)

のように引数でコピーが発生する。どっちが速いかはきっと大差ないけど、メモリー使用量を無駄に増やすことはないと思う。ポインタ経由で渡そう。

無意味なstrcpy_s

抜粋すると

char sex[16];
if (data.sex == 1)
{
    strcpy_s(sex, 16,"男性");
}
else
{
    strcpy_s(sex, 16,"女性");
}
printf("性別：%s\n",sex);

こんなコードが見えるが、三項演算子を使うべき場面であって、strcpy_sを使うべき理由はない。

不正な入力に対するエラー処理

初心者本に求めるのは間違っているが、不正な入力に対する対策がない。

それunsignedにできるよ

無味に符号付き整数型を使うような愚策はGoogle C++ Style Guideにでも任せておけばいい。そんな愚行に付き合う必要はない。

符号がないことでバグを作り込む確率を大幅に下げられる。詳しくは
C/C++はnull安全になる前に安全に差の絶対値を計算できるようになるべきではないか
を参照して欲しい。

分裂したprintf

たしかに

printf("名前：%s\n",data.name);
printf("年齢：%d\n",data.age);

のように分けたほうが見やすいと主張する人がいるのは知っている。でも

printf(
    "名前：%s\n"
    "年齢：%d\n",
    data.name, data.age
);

くっつけたほうが見やすいと私は思う。

printf("\n");

printfを使うべき理由がない。putcharで行こう。

結果

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>
#include <errno.h>
#include <stdbool.h>
#include <ctype.h>
#include <float.h>
#include <assert.h>
#ifndef static_assert
#   define NO_STDC_STATIC_ASSERT
#   define static_assert(...)
#endif
#if defined(_MSC_VER) || defined(__cplusplus)
#   define restrict
#endif
/**
 * @brief 文字列が文字を持っているか調べます。
 * @param str 対象文字列へのポインタ
 * @return false: nullptrか空白文字のみの文字列 true:それ以外
 */
static inline bool str_has_char(const char *str)
{
    if (NULL == str) return false;
    bool ret = false;
    for (; !ret && *str != '\0'; str++) ret = (*str != ' ');
    return ret;
}
/**
 * @brief 文字列が文字を持っているか調べます。
 * @param io 書き換えるbool型変数へのポインタ、呼び出し後はポインタが指す変数にnew_valueが代入される
 * @param new_value 新しい値
 * @return ioが指すbool変数がもともと持っていた値
 */
static inline bool exchange_bool(bool* restrict const io, const bool new_value)
{
    const bool tmp = *io;
    *io = new_value;
    return tmp;
}
/**
 * @brief fgetsで失敗したときにストリームをクリアしてループする関数
 * @param s ストリームから読み取った文字列を格納するための領域へのポインタ
 * @param buf_size ストリームから読み取った文字列を格納するための領域の大きさ
 * @param stream FILE構造体へのポインタかstdin
 * @param message_on_error エラー時に表示してループする
 * @return 成功時は0, new line at the end of fileのときは-1
 */
static inline int fgets_wrap(char* restrict const s, size_t buf_size, FILE* restrict const stream, const char* restrict message_on_error)
{
    size_t i = 0;
    for (bool first_flg = true; i < 100 && NULL == fgets(s, buf_size, stream); ++i) {
        if (feof(stdin)) return -1;
        if (!exchange_bool(&first_flg, false)) puts((message_on_error) ? message_on_error : "再入力してください");
    }
    if (100u == i) exit(1);//無限ループ防止
    if (feof(stdin)) return 0;
    //改行文字が入力を受けた配列にない場合、入力ストリームにごみがある
    const size_t len = strlen(s);
    //短すぎる入力
    if (0 == len || (1 == len && '\n' == s[0])) return 1;
    //長過ぎる入力
    if ('\n' != s[len - 1]) {
        //入力ストリームを掃除
        while (fgetc(stream) != '\n');
        return 2;
    }
    return 0;
}

/**
 * @brief 標準入力から文字列の入力を受ける
 * @param s ストリームから読み取った文字列を格納するための領域へのポインタ
 * @param buf_size ストリームから読み取った文字列を格納するための領域の大きさ
 * @param message 入力を受ける前にputsに渡す文字列。表示しない場合はnullptrか空白文字のみで構成された文字列へのポインタを渡す
 * @param message_on_error エラー時に表示してループする
 */
static inline void input_str(char* restrict const s, size_t buf_size, const char* message, const char* restrict message_on_error)
{
    if (str_has_char(message)) puts(message);
    size_t i = 0;
    for (; i < 100u; ++i) {
        //長過ぎる入力以降の無限ループ防止にerrnoをクリアする
        errno = 0;
        switch (fgets_wrap(s, buf_size, stdin, message_on_error)) {
        case -1: return;//EOF
        case 1://短すぎる入力
        case 2://長過ぎる入力
            continue;
        default: goto after_loop;
        }
    }
    if (100u == i) exit(1);//無限ループ防止
after_loop:
    {
        char* const lf = strchr(s, '\n');
        if(NULL != lf) lf[0] = '\0';
    }
}

/**
 * @brief 標準入力から入力を受け、unsigned int型に変換する
 * @details fgetsしてstrtodしている。max, minの条件に合わないかエラー時はループ
 * @details errnoの値を書き換える
 * @param message 入力を受ける前にputsに渡す文字列。表示しない場合はnullptrか空白文字のみで構成された文字列へのポインタを渡す
 * @param message_on_error エラー時に表示してループする
 * @param max 入力値を制限する。最大値を指定
 * @param min 入力値を制限する。最小値を指定
 * @return 入力した数字、EOFのときは0
 */
static inline unsigned int input_uint(const char* message, const char* restrict message_on_error, const unsigned int max, const unsigned int min)
{
    if (str_has_char(message)) puts(message);
    char s[30];
    static_assert(sizeof(unsigned int) < 8, "err");
    unsigned long t = 0;
    size_t i = 0;
    for (char* endptr = s; ((0 == t && endptr == s) || 0 != errno || t < min || max < t) && i < 100u; ++i) {
        //長過ぎる入力以降の無限ループ防止にerrnoをクリアする
        errno = 0;
        switch (fgets_wrap(s, sizeof(s), stdin, message_on_error)) {
        case -1: return 0;//EOF
        case 1://短すぎる入力
        case 2://長過ぎる入力
            endptr = s;//ループ制御フラグとして流用
            continue;
        default: break;
        }
        t = strtoul(s, &endptr, 10);
    }
    if (100 == i) exit(1);//無限ループ防止
    return ((unsigned int)(t));
}
#ifdef NO_STDC_STATIC_ASSERT
#   undef static_assert
#   undef NO_STDC_STATIC_ASSERT
#endif

#ifndef _countof
#define _countof(arr) (sizeof(arr) / sizeof(*arr))
#endif

typedef struct
{
    char name[256];
    unsigned int age;
    unsigned int sex;
}People;

void InputPeople(People *data)
{
    do {
        input_str(data->name, _countof(data->name), "名前を入力してください", "不正な長さの入力です、もう一度入力してください");
        printf("%sでよろしいですか？？（「合ってます」：「1」を入力、「間違いました」：「1以外」を入力）\n", data->name);
    } while (!feof(stdin) && 1 != input_uint(NULL, NULL, UINT_MAX, 0) && !feof(stdin));
    if (feof(stdin)) exit(1);
    do {
        data->age = input_uint("年齢を入力してください", NULL, UINT_MAX, 0);
        printf("%dでよろしいですか？？（「合ってます」：「1」を入力、「間違いました」：「1以外」を入力）\n", data->age);
    } while (!feof(stdin) && 1 != input_uint(NULL, NULL, UINT_MAX, 0) && !feof(stdin));
    if (feof(stdin)) exit(1);
    do {
        data->sex = input_uint("性別を入力してください（「男性」：「1」を入力、「女性」：「2」を入力）", NULL, 2, 1);
        printf("%sでよろしいですか？？（「合ってます」：「1」を入力、「間違いました」：「1以外」を入力）\n", (1 == data->sex) ? "男性" : "女性");
    } while (!feof(stdin) && 1 != input_uint(NULL, NULL, UINT_MAX, 0) && !feof(stdin));
    putchar('\n');
}
void ShowPeople(const People* data)
{
    printf(
        "名前：%s\n"
        "年齢：%d\n"
        "性別：%s\n\n",
        data->name, data->age, (data->sex == 1) ? "男性" : "女性"
    );
}

int main(void)
{
    People data[3];
    for (size_t i = 0; i < 3; i++) {
        if (feof(stdin)) exit(1);
        InputPeople(&data[i]);
    }
    for (size_t i = 0; i < 3; i++) {
        ShowPeople(&data[i]);
    }
    return 0;
}

https://wandbox.org/permlink/s4XjyyQWnTpgmINS

restrictキーワードの使い方いまいちわかってない勢なのでそこは許して下さい。