C言語でHit&Blowを作りました。評価や代案が欲しいです！

Question

C言語でエラーなく作ることが出来たのですが皆さんの意見が聞きたいです。
勉強中なので出来る人からしたらの素直な意見が知りたいです。
（ポインタを使う予定でしたが自分にはまだまだ理解できてないので使いませんでした）
特に　void nums(int num[]) の感想が知りたです

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

#define len 3

void randint(int ran[]); //乱数による重複しない数値を設定
void nums(int num[]);      // 標準入力を格納
int Hit(int ran[],int num[]);     //同じ位置にranとnumの数値チェック
int Blow(int ran[],int num[]);     //異なる位置にranとnumの数値チェック

int main(int argc, const char * argv[]) {
    // insert code here...
    int N[len],val[len];
    int cnt;
    
    printf("10回以内に%dつの数値を All Hit して下さい。\n\n",len);
  
    randint(N);
    
    for(cnt = 0; cnt < 10; cnt++){
        printf("%d回目ー＞ ",cnt+1);
        nums(val);
        
        int Hit_cnt = Hit(N,val);
        int Blow_cnt = Blow(N,val);
        
        if(Hit_cnt == len){
            printf("All Hit !!\n");
            break;
        }
        printf("Hit ->%2d　Blow ->%2d\n\n",Hit_cnt,Blow_cnt);
    }
    
    if(cnt == 10){
        printf("正解は[");
        for(int i = 0; i < len; i++){
            printf("%2d",N[i]);
        }
        printf(" ]です。\n");
    }
    
    return 0;
}

void randint(int ran[]){
    int x,y;
    
    srand((unsigned)time(NULL));
    
    for(x = 0; x < len; x++){
        do{
            ran[x] = rand() % 10;
            for(y = 0; y < len; y++){
                if(ran[x]==ran[y]){
                    break;
                }
            }
        }
        while(x != y);
    }
}

void nums(int num[]){
    char val[100];
    int num_cnt;
    
    for(int loop=0;;loop++){
        scanf("%s",&val,100);
        int val_cpy = (int)val;
        num_cnt = 0;
        
       
        
        if(strlen(val) == len){
            for(int i = len-1; i >= 0; i--){    //num配列に代入
                num[i] = val_cpy % 10;
                val_cpy /= 10;
            }
            break;
        }
        else{
            printf("やり直しです。 %d文字入力して下さい。 -> ",len);
        }
    }
}

int Hit(int ran[],int num[]){
    int i,Hit_cnt = 0;
    
    for(i = 0; i < len; i++){
        if(ran[i] == num[i]){
            Hit_cnt++;
        }
    }
    
    return Hit_cnt;
}

int Blow(int ran[],int num[]){
    int Blow_cnt = 0;
    
    for(int x = 0; x < len; x++){
        for(int y = 0; y < len; y++){
            if(x != y && ran[x] == num[y]){
                Blow_cnt++;
            }
        }
    }
    
    return Blow_cnt;
}

Comments

[jimbe]

>エラーなく作ることが出来た

コンパイルエラーが無ければ完成…ではありません。
あらゆる場面において想定通りの動作したら完成です。
ご提示のコードは、果たしてそのように動作するのでしょうか。

[saruganseki]

返答遅くなりました
一応、実行してみて問題なく自分が描いた通りには動いています。
ただ、プログラミングできる人から見てどう思うのか。何を考えれば良いのかが
知りたかったのです

[kazuma-s]

> 一応、実行してみて問題なく自分が描いた通りには動いています。

その動いているコードを質問に追記してください。そうすれば
「見てどう思うのか。何を考えれば良いのか」をお伝え出来ます。

Answer 1

関数:nums は正しく動作してますか?
int val_cpy = (int)val; がものすごく怪しいのですが...

break; の直前に　
printf("[%d %d %d]\n", num[0], num[1], num[2]); して
入力値が期待通りに格納されているか確認してください。

[追記]

ちなみに、epistemeさんならvoid nums(int num[])文はどの様に作るか教えて頂けませんか？

#include <stdio.h>
#include <string.h>
#include <ctype.h> 
#include <stdbool.h>

void nums(int num[], int len) {
  bool ok = false;
  while ( !ok ) {
    char val[100];
    scanf_s("%s",val,100);

    ok = (strlen(val) == (size_t)len);
    for ( int i = 0; i < len && ok; ++i ) {
      ok = isdigit(val[i]);
      num[i] = val[i] - '0';
    }
    if ( !ok ) {
      printf("やり直しです。 %d文字入力して下さい。 -> ",len);
    }
  }
}

int main(void) {
  int num[3];
  nums(num, 3);
  printf("[%d %d %d]\n", num[0], num[1], num[2]);
  return 0;
}

Comments

[saruganseki]

返事遅くなりました
一応、問題なく実行できます（自分の描いた通りに動きます）。なぜ、このようになったかと言うと
当初は
void nums(int num[]){
int val;
int num_cnt;

for(int loop=0;;loop++){
scanf("%d",&val);
int val_cpy = val;
num_cnt = 0;

while(val != 0){ //要素数０番目が０の場合インクリメントできない
val /= 10;
num_cnt++;
}

if(num_cnt == len){
for(int i = len-1; i >= 0; i--){ //num配列に代入
num[i] = val_cpy % 10;
val_cpy /= 10;
}
break;
}
else{
printf("やり直しです。 %d文字入力して下さい。 -> ",len);
}
}
}
でした。仮に" 012 " と標準入力すると０がカウントされない事に気づいたので自分なりの
シンプルな考えであるchar型で取得して文字列を無理矢理、整数に変えています

[episteme]

> 一応、問題なく実行できます（自分の描いた通りに動きます）
char val[] = "123";
int val_cpy = (int)val;
すると val_cpy に 123 が得られるんですか? んなわけないんだけど。

[saruganseki]

int main文での
nums(val); の下にfor文によるprintf("nums[%d] -> %d", i, val[ i ]);
と記述した際は自分が標準入力したのと同じものが返ってきました。
int型　を　(double)変数名　で変換できるのを教わっていたので
文字列を無理矢理、整数として要素数に一個ずつ入れられるかな？という自分なりのシンプルな
考えでやってみました
ちなみに、epistemeさんならvoid nums(int num[])文はどの様に作るか教えて頂けませんか？

[episteme]

追記しました。

[saruganseki]

追記ありがとうございます。
まだ習った事のないものがありましたが勉強になります
while( !ok ) は　okがfalseではない　と言う否定するものだった様な・・・。
自分にはまだまだ基礎ができていないみたいなので勉強します

[episteme]

> while( !ok ) は　okがfalseではない

逆です。okじゃない(not true)間くりかえし、です。

Answer 2

入力とチェックを分けるとスッキリすると思います。
ついでには、果たして num 配列(や同様の変数)は int である必要があるのかどうか…。

int isValidNumbers(char *numbers, int requiredSize) {
  if(strlen(numbers) != requiredSize) return 0; //過不足
  for(int i=0; i<requiredSize; i++) {
    if(numbers[i] < '0' || '9' < numbers[i]) return 0; //数字じゃない
    for(int j=0; j<i; j++) {
      if(numbers[j] == numbers[i]) return 0; //同じ数字が2度以上
    }
  }
  return 1; //有効
}

void nums(int num[]) {
  char val[100];
  while(1) {
    scanf("%s", val);
    if(isValidNumbers(val, len)) break;
    printf("やり直しです。 %d文字入力して下さい。 -> ",len);
  }
  for(int i=0; i<len; i++) num[i] = val[i] - '0';
}

Answer 3

まず注意しておきたいのは Teratail では質問は具体的にすることが推奨されます。 要するに解決すべき問題がわからない、何を答えれば終わるのかがわからないのは悪い質問です。 感想が欲しいというような曖昧な要望は避けてください。

質問のコツはヘルプとしてまとめられているのでまずは一読することをお勧めします。

https://teratail.com/help/question-tips

流儀は色々

コーディングスタイルには色々な流儀があり、それぞれに利点も欠点もあります。 ほとんどの人が賛成する流儀もあれば意見が別れるものもあり、明らかに仕様に反する駄目な場合や意図通り動かない場合を除けばどういう選択が望ましいのかはとても微妙な判断の連続です。

時代によってもその他の様々な環境の事情によっても変わってくるので教条的に覚えていくのではなく背景にある理由を学んできちんと選択できるようにしなければなりません。 結局は経験を積み重ねるしかないことなので言語仕様の基礎が十分に理解できていない状況でこの場合についての個別の指摘を貰ってもあまり身につかないのではないかという気がします。

ポインタについて

ポインタを使う予定でしたが自分にはまだまだ理解できてないので使いませんでした

関数の仮引数が配列形式で宣言されるとポインタに調整されるという特別な規則があります。 void nums(int num[]); と書くと void nums(int *num); と書いたのと同じ意味です。 また、式中に現れる配列は原則として (つまり例外もあるということですが) 配列の先頭要素を指すポインタに変換されます。 つまり nums(val); と書いている箇所では val はポインタとして渡されています。

一見すると配列を渡して配列を受け取っているように見えますが、渡す側と受け取る側で暗黙にポインタへと調整されているので、ポインタとしてのやり取りなのです。 直接的にポインタを宣言してなくてもポインタが現れることはあります。

C ではインデクスアクセスすらポインタ演算の構文糖です。 num[i] は *(num+i) と同等だと解釈されます。 正確な理解のためにはポインタは避けて通れません。

マクロ名について

rubato6809 さんの回答でも述べられているようにマクロ名はすべて大文字にする (かつマクロ名以外では全て大文字の名前は使わない) という習慣があります。 マクロはプログラムの処理の序盤で字句的に置き換えるので通常の変数名などとは解釈が違うということから、マクロはマクロであるとわかるようにしていたほうが良いという考え方です。

仕様上の規則ではないですし、命名には様々な流儀があるのでこれが常に良い方法だと考えられているわけでもないということは断っておきますが、いずれにしてもマクロはちょっと違うということは意識するとよいでしょう。

// 変数名がかぶっていたらより狭いスコープのほうだと解釈される
#include <stdio.h>

int foo = 1;

int main(void) {
  int foo = 2;
  printf("%d\n", foo); // 2 が表示される
}

似たような構造をしていても ↓ はエラーになります。

#include <stdio.h>

#define foo 1

int main(void) {
  int foo = 2;
  printf("%d\n", foo);
}

何故なら上記のマクロはまず

#include <stdio.h>

int main(void) {
  int 1 = 2;
  printf("%d\n", 1);
}

のように展開されてしまうので文法的におかしなことになるからです。

失敗時の処理

関数内でなんらかの失敗があった場合には失敗したという結果を (返却値で) 返し、関数内でリトライはしないのが通例です。

失敗の対処としてやり直すのか、エラーを出して終了するのか、デフォルト値を適用するのかといった選択肢が考えられ、関数の中で決め打ちしてしまうとそれ以外の選択ができなくなってしまうからです。

Comments

[saruganseki]

沢山の意見ありがとうございます。
ポインタについてー＞やはり避けてはダメですね。
現在、参考書やネットで学ぶだけでなく写経を何回も繰り返して感覚を覚える事もしているので
これからも頑張ってみます

Answer 4

コード・レビューは色々お伝えできるので私は歓迎です。nums() 関数だけで結構ありました。関数の先頭からコメントしていきます。

・void nums(int num[]) は例えば void userInput(int num[]) に

関数名は少し工夫しましょう。何をする関数か示すようにした方がよい。私自身もあまりセンス良く無いのだけど inputNumber()、userInput() みたいな関数名を挙げておきます。

・ char val[100]; は例えば char buff[100]; に

変数名も工夫しよう。配列名 val は「値・整数値」から連想したと思いますが、scanf("%s" で入力するのは文字列であり、数字でない文字も入力できるので、val はどちらかというと相応しくない感じ。buff は一例。

・int num_cnt; は削除

num_cnt に０を代入してますが、それ以外に何も使われていない、不要な変数です。

・for (int loop=0;;loop++) は for (;;) か while (1)

この for ループは正しく３桁入力できるまで繰り返す無限ループですが、ループの中で loop 変数を使っていないので、loop変数は不要です。

・scanf("%s", &val, 100); は scanf("%s", val); 又は scanf_s("%s", val, 100);

ここに２点あります。"%s" で文字列を入力するのですが、
まず、２番目の引数（文字列が格納される場所）を &val としている事。これは間違いではありませんが、単に scanf("%s", val); とするだけで良いです。配列名 val は配列 val の先頭アドレスです。&val[0]と同じです。
もう一つ、「100」という引数（配列のサイズ）を渡してますが、これが必要なのは scanf_s() 関数だと思います（scanf() には不要な引数）。さて、どちらなんだろう？

取り急ぎ見つけたページです。
scanf_sをscanfの代わりに使うときのやりかた

・int val_cpy = (int)val; はバグ

配列 val[] に格納されているのは文字列・数字列です。例えば "123" なら '1', '2', '3' という３文字です（４文字目は '\0' かな）。
それを数値 123 として扱いたいのですが、(int) でキャストしても 123 という数値に変換されません（変換するなら普通 atoi()、strtol() 等を使う）。

では val_cpy = (int)val; は何か。
配列名 val は配列 val の先頭アドレスです。念のため：メモリアドレスです。配列に格納されている文字でもデータでもありません。
メモリアドレスを int 型にキャストすれば int 型変数に代入する事はできますが、代入される値はあくまでもメモリアドレスです。期待する値ではないのだから正しく動作しません。HIT/BLOW の値は間違ったものになるという事。

変数が割当てられるのはメモリですが、メモリのアドレスとデータをごっちゃにしてはいけない。ポインタを使えてない段階なら、ここに改めて問いを立ててもよいかもです。

・ニ行以上続く空行は無意味です。空行は一行だけにする。

・外側ループの中、if () { ... break; } else { printf(); } はelseを削除する

break でループから脱出してしまえば、その直後の処理を実行することはありません。なので else を省略できます。その分、インデントを浅くでき、ループの構造が分かりやすくなります。こんな格好です。

    for (...) {
        ...
        if (条件) {
            ...
            break;
        }
        printf(...); 

・#define len 3 は #define LEN 3　に
・if (strlen(val) == len) { は if (strlen(val) == LEN) { に

これは文法規則でなく、コーディングスタイルの問題です。
マクロには大文字を使うのが好ましい。小文字だと変数名かな？と勘違いしやすい。大文字なら見ただけで「変数じゃなさそうだ、マクロかな」と判断できる。
逆に main() 関数の中の int N[len] も好ましくありません。

・（break 直前の）内側の for ループは外に出したほうが良いのでは。
内側の for ループは入力した３つの数を、呼出し側の配列に格納して返すものです。再入力を繰り返す外側のループが何回繰り返しても、格納する処理は関数が終了する直前の一回だけです。その一回の処理は外側のループを抜けた後で構わないし、またそうした方が「再入力を繰り返すループ」処理が簡潔になります。
こんな感じ。

    // 正しく入力できるまで繰り返す
    for (;;) {
        scanf("%s", buff);
        if (strlen(buff) == LEN)
            break;               // （ここはループを脱出するだけ）

        printf("やり直しです。 %d文字入力して下さい。 -> ", LEN);
    }

    // 呼出し元にコピー
    for (int i = 0; i < LEN; i++)
        ....

評価
バグがあります。出直しましょう。エラーなく作ることが出来た…でもテストもデバッグもできていない。コンパイルエラーが出なくなっただけなら、ようやく入口に立ったにすぎません。

Comments

[saruganseki]

返答遅くなりました。長文な上、細かい指摘は本当に嬉しいです！

変数名の指摘ー＞その時の自分から見たらわかるけど、数ヶ月後には見たら見たらすぐにわかるのか？周りが見ても理解できる？っと言う事なのかなと思いました。・・・となると英語の勉強もやった方がいいなぁ。

無限ループの書き方ー＞今までこの様な書き方っだったの「確かにそっちの方が見やすい（理解しやすい）」と思いました。変数を宣言してる以上、何かに使うと思っているところもあったのですぐにマネさせていただきます。

scanfとscanf_sの違いー＞初めに学校ではVisual Studio を使っていますが家ではxcodeを使っています。文字列を標準入力する際は配列のサイズを書く必要があるという考えだったので知りませんでした。
学校ではscanf_s 家ではscanf　でやっていますがエラー表示が出たらscanf_sに変えるという認識でした

コーディングスタイルー＞#define LEN 3　のように”大文字”にしたらいい。・・・これだけの意見でも
意識の仕方が変わりますし聞けて嬉しいです。

まだまだ初心者だなと痛感しました早く中級者になれるよう改善します