[C言語]このコードをより良いものにしてほしいです。

Question

独学で「C言語プログラミングワークブック」という本を読んでいるのですが、その中にある課題の答えが本に載っていません。googleやgithubで調べても載っていません。もちろんこれは学校の課題などではないです。

以下課題と自分の解答です

課題

5人の選挙候補者（番号１から５）への20人による投票データが次のように与えられているものとします。
1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3
ただし、この数字は候補者の番号で、0は白票を意味します。このデータから、

1. それぞれの候補者および白票の票数を集計して表示し、
2. 最大の得票数を得た候補者番号（１～５、複数いればすべて）を表示する。

自分の答え

コード

#include<stdio.h>

int main(void)
{
    int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
    int b;
    int c,d,e,f,g,h;
    c=0;
    d=0;
    e=0;
    f=0;
    g=0;
    h=0;
    int i;
    

    for (b = 0; b < 20; b++) {
        if (a[b] == 0) {
            c++;
        }
        if (a[b] == 1) {
            d++;
        }
        if (a[b] == 2) {
            e++;
        }
        if (a[b] == 3) {
            f++;
        }
        if (a[b] == 4) {
            g++;
        }
        if (a[b] == 5) {
            h++;
        }
    }
    printf("白票は%d 票です。\n1の票数は%d 票です。\n2の票は%d 票です。\n３の票は%d 票です。\n４の票は%d 票です。\n５の票は%d 票です。\n",c,d,e,f,g,h);
    
    return 0;
}

ターミナル

白票は2 票です。
1の票数は4 票です。
2の票は4 票です。
３の票は3 票です。
４の票は4 票です。
５の票は3 票です。

似たif文を6個も並べているので省略したいですが、解決策が思い浮かびません。
2.に関しては回答すら思いつきません😣
また、初学者なのでコードが読みにくい等あるかもしれませんので、改善点があれば教えていだたきたいです。

追記

回答者様のご指摘のおかげで動作できるまでになりましたので、そのコードを載せておきます！

コード

#include<stdio.h>

int main(void)
{
    int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
    int b;
    int hyo[6] = {0,0,0,0,0,0};
    int x;
    int i;

    for (b = 0; b < 20; b++) {
        hyo[a[b]]++;
    }
    printf("白票は%d 票です。\n",hyo[0]);
    for (x = 1; x < 6; x++) {
        printf("候補者番号%d は%d 票です。\n", x, hyo[x]);
    }
    int max = hyo[1];

    for (i = 2; i < 6; i++) {
        if (hyo[i] > max) {
            max = hyo[i];
        }
    }
    for (i = 1; i < 6; i++) {
        if (hyo[i] == max) {
            printf("候補者番号%d は、最大の得票数を得ました。\n", i);
        }
    }

    return 0;
}

ターミナル

白票は2 票です。
候補者番号1 は4 票です。
候補者番号2 は4 票です。
候補者番号3 は3 票です。
候補者番号4 は4 票です。
候補者番号5 は3 票です。
候補者番号1 は、最大の得票数を得ました。
候補者番号2 は、最大の得票数を得ました。
候補者番号4 は、最大の得票数を得ました。

Comments

[dfbgs]

テスト

Answer 1

関数化したり変数名を変えたり等。

#include <stdio.h>
#include <memory.h>

int totalling(int *vote, int vote_size, int *c_votes, int c_votes_size) {
  //クリア
  memset(c_votes, 0, sizeof(c_votes[0]) * c_votes_size);
  //集計, 最大得票数
  int max = -1;
  for(int i=0; i<vote_size; i++) {
    int candidate = vote[i];
    if(candidate < 0 || c_votes_size <= candidate) candidate = 0; //範囲外は無効票
    c_votes[candidate] ++;
    if(candidate != 0 && c_votes[candidate] > max) max = c_votes[candidate]; //candidate=0 は無効票のため,最大を求める対象にしない
  }
  
  return max;
}

void output(int *c_votes, int c_votes_size, int max) {
  printf("白票は%d 票です。\n", c_votes[0]);
  for(int i=1; i<c_votes_size; i++) {
    printf("候補者番号%d は%d 票です。\n", i, c_votes[i]);
  }

  for(int i=1; i<c_votes_size; i++) {
    if(c_votes[i] == max) {
      printf("候補者番号%d は、最大の得票数を得ました。\n", i);
    }
  }
}

#define NUMBER_OF_CANDIDATES 5 //候補者数

int main(void) {
  int vote[] = {1,2,5,2,3,4,0,1,1,5, 3,4,4,2,5,4,0,1,2,3};
  int c_votes[NUMBER_OF_CANDIDATES+1];

  int max = totalling(vote, sizeof(vote)/sizeof(vote[0]), c_votes, NUMBER_OF_CANDIDATES+1); 

  output(c_votes, NUMBER_OF_CANDIDATES+1, max);

  return 0;
}

Comments

[dfbgs]

jmbeeさん回答ありがとうございます。
私にはまだわからない表現ばかりですが、いつかこのようなコードが書けるように精進したいと思います。
模範的な解答ありがとうございます！

Answer 2

配列を学んだばかりなんですね。

似たif文を6個も並べているので省略したい

得票数の変数 c ～ h を配列にするのです。otnさんに倣って、
・hyo[6] という配列を設ける。ただし、hyo[6] の初期値が全て０にする必要があります。
c が hyo[0] に、d が hyo[1] に、、、h が hyo[5] に対応しますので、a[b] の値は hyo[] 配列の添え字（インデックス）として使えます。すると

       x = a[b];   // 票の番号を得て
       hyo[x]++;   // その人の得業を＋１する

この2行は hyo[a[b]]++; とまとめられます。これを a[0] ～ a[19] まで繰り返せば（＝ループすれば）よい。

2.に関しては回答すら思いつきません

配列を学んだ頃に、与えられた配列から最大値（最小値）を探せ、という例題か課題がありそうなものです。

（a） 上の処理を行えば hyo[] 配列に得票数が並びます（得票数が全て確定する）ので、hyo[0]～hyo[5] を順に見ていけば（＝ループすれば）、最大得票数を探すことができます。この時、暫定最大数と言える変数（例えば max）を設けておき、max より hyo[i] の方が大きかったら max = hyo[i]; とします（暫定最大数を更新する）。ループを終了した時の max が最大数です。
次に、改めて hyo[] を先頭からみて（ループして）、最大得票数 max と一致する添え字（インデックス）を表示できます。

配列から最大値・最小値を求める基本はこうですが、この問題だと

(b) hyo[] 配列の値を＋１する最初のループで、同時に最大得票数を求められます。＋１した hyo[i] と暫定最大数 max を比較し、hyo[i] が大きかったら max を更新するのです。
hyo[] の値が全て確定した時点で最大得票数 max も確定します。そこで改めて hyo[] 配列を先頭からみて（ループして）、max と一致する添え字（インデックkス）を表示できます。
Enjoy ！

Comments

[dfbgs]

大変ご丁寧な説明ありがとうございます！
hyo[a[b]]++;という省略の仕方があるんですね。勉強になります。便利ですね！
最大値の求め方を丁寧に解説してくださりありがとうございます。
参考にさせていただきます！

Answer 3

似たif文を6個も並べているので省略したいですが、解決策が思い浮かびません。

配列を使います。
int hyo[6] = {0,0,0,0,0,0};
として、x番さんの票数をhyo[x]でカウントします。

2.に関しては回答すら思いつきません😣

hyo[i](iは1,2,3,4,5)の中で最大の物を求めて、その最大数と等しいhyo[i]を持つi(複数該当あり)が最大得票者です。

Comments

[dfbgs]

できました😭
ありがとうございます！

[dfbgs]

## コード
````
#include<stdio.h>

int main(void)
{
int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
int b;
int hyo[6] = {0,0,0,0,0,0};
int x;
int i;
int max = hyo[1];

for (b = 0; b < 20; b++) {
x = a[b];
if (a[b] == x) {
hyo[x]++;
}
}
printf("白票は%d 票です。\n1の票数は%d 票です。\n2の票は%d 票です。\n３の票は%d 票です。\n４の票は%d 票です。\n５の票は%d 票です。\n",hyo[0],hyo[1],hyo[2],hyo[3],hyo[4],hyo[5]);

for (i = 2; i < 6; i++) {
if (hyo[i] > max) {
max = hyo[i];
}
}
for (i = 2; i < 6; i++) {
if (hyo[i] == max) {
printf("候補者番号%d は、最大の得票数を得ました。\n", i);
}
}

return 0;
}
````
## ターミナル
````
白票は2 票です。
1の票数は4 票です。
2の票は4 票です。
３の票は3 票です。
４の票は4 票です。
５の票は3 票です。
候補者番号2 は、最大の得票数を得ました。
候補者番号4 は、最大の得票数を得ました。
````

[rubato6809]

質問者が作り直したコードには大きな進展が見られて喜ばしいのですが、いくつか指摘したいことがあります。その中で最大の問題は
int max = hyo[1];　という辺りが明らかなバグだという事。おそらく勘違いがあると思います。

そのデータだと不具合に気づかないでしょうが、hyo[1] が最大得票数となる場合、それを正しく表示できない事が重大問題です。hyo[1]が最大得票になるテストデータを用いてテストしなおし、不具合を確認されることを強くお勧めします。

[jimbe]

> x = a[b];
> if (a[b] == x) {

修正前のコードを引きずっているのでしょうか。
この 2 行の関係を考えてみてください。
1 行目で x に a[b] を代入していますので 2 行目の if 文は必ず成立しますから、そもそも if 文は必要ありません。

[dfbgs]

robato6809さん。わざわざ回答ありがとうございます。
for (b = 0; b < 20; b++) {
hyo[a[b]]++;
}
とhyo[1]に値を代入した後に、int max = hyo[1];を定義しないといけないということかなと思いました。

jimbeさんご回答ありがとうございます。
確かにそうですね。わざわざご指摘ありがとうございます。

最後に一応変更したコードを載せておきます。
````
#include<stdio.h>

int main(void)
{
int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
int b;
int hyo[6] = {0,0,0,0,0,0};
int x;
int i;

for (b = 0; b < 20; b++) {
hyo[a[b]]++;
}
printf("白票は%d 票です。\n1の票数は%d 票です。\n2の票は%d 票です。\n３の票は%d 票です。\n４の票は%d 票です。\n５の票は%d 票です。\n",hyo[0],hyo[1],hyo[2],hyo[3],hyo[4],hyo[5]);

int max = hyo[1];

for (i = 2; i < 6; i++) {
if (hyo[i] > max) {
max = hyo[i];
}
}
for (i = 2; i < 6; i++) {
if (hyo[i] == max) {
printf("候補者番号%d は、最大の得票数を得ました。\n", i);
}
}

return 0;
}
````

robato6809さん、jimbeさん改めてわざわざご回答ありがとうございました！

[rubato6809]

printf("白票は%d 票です。\n1の票数は…
ここ間違いではないですが、得票数が配列になったことを活かし、ループで表示させることも考えてみたらどうでしょうか。

[dfbgs]

回答ありがとうございます！
````
printf("白票は%d 票です。\n1の票数は%d 票です。\n2の票は%d 票です。\n３の票は%d 票です。\n４の票は%d 票です。\n５の票は%d 票です。\n",hyo[0],hyo[1],hyo[2],hyo[3],hyo[4],hyo[5]);
````
この部分を
````
printf("白票は%d 票です。\n",hyo[0]);
for (a[b] = 1; a[b] < 6; a[b]++) {
printf("候補者番号%d は%d 票です。\n", a[b], hyo[a[b]]);
}
````
このように書き換えました！

## コード
````
#include<stdio.h>

int main(void)
{
int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
int b;
int hyo[6] = {0,0,0,0,0,0};
int x;
int i;

for (b = 0; b < 20; b++) {
hyo[a[b]]++;
}
printf("白票は%d 票です。\n",hyo[0]);
for (a[b] = 1; a[b] < 6; a[b]++) {
printf("候補者番号%d は%d 票です。\n", a[b], hyo[a[b]]);
}
int max = hyo[1];

for (i = 2; i < 6; i++) {
if (hyo[i] > max) {
max = hyo[i];
}
}
for (i = 2; i < 6; i++) {
if (hyo[i] == max) {
printf("候補者番号%d は、最大の得票数を得ました。\n", i);
}
}

return 0;
}
````
## ターミナル
````
白票は2 票です。
候補者番号1 は4 票です。
候補者番号2 は4 票です。
候補者番号3 は3 票です。
候補者番号4 は4 票です。
候補者番号5 は3 票です。
候補者番号2 は、最大の得票数を得ました。
候補者番号4 は、最大の得票数を得ました。
````

[rubato6809]

for (a[b] = 1; a[b] < 6; a[b]++) {
動作したとしても、これは明らかな間違いです。
バッファオーバーランという性質（たち）の悪いバグになっています。

[dfbgs]

すみません、、、バッファオーバーランについてはぐぐって概念を理解したのですが、なぜバクなのかは正直わかりません、、、
````
for (a[b] = 1; a[b] < 6; a[b]++) {
````
この部分を
````
x = a[b];
for (x = 1; x < 6; x++) {
printf("候補者番号%d は%d 票です。\n", x, hyo[x]);
}
````
このように書きかえたらいいでしょうか？

[rubato6809]

x = a[b];　は不要です。そしてこれもバッファオーバーラン（範囲外アクセス）です。
for (x = 1; で変数 x に１を代入してループを開始するのだから x = a[b]; とする必要はないのです。

for (x = 1; x < 6; x++) {　とする時の変数 x をループカウンタなどと呼びます。
for (a[b] = 1; a[b] < 6; a[b]++) {　と見比べると、x の役目を、代りに a[b] に担わせたのです。ですが、a[b] を使う必要はなく、ループカウンタには単純変数 x を使えば十分です。というか、こうするのが基本です。

int a[20]; と変数定義した配列 a[] は、a[0] ～ a[19] の２０個（20要素）がメモリに割り当てられています。しかし a[20] はメモリに正当に割当てられていません。
ところが、for (a[b] = 1; を実行する時点の b の値は20になっています（なぜなら、その直前の for ループで b が20になった時点でループが終了したから）。
つまり、その a[b] とは a[20] ですが、メモリ上に正当に割当てられていない a[20] をループカウンタとして使おうとした・・・即ち「範囲外アクセス」＝「バッファオーバーラン」というわけです。Ｃ言語は簡単にこういう間違いをおこせる言語なので注意が必要です。

a[20] をアクセスしてプログラムの動作に支障がなかったとしても、単に幸運だっただけです。もしそこが別の目的に使われている変数だったなら、何等かの動作不具合を起こす可能性があります。具体的な不具合はどうなるか決まっていません。ただちにプログラムが停止することもある、何も不具合を起こさないこともある、書き変えた時点から少し時間が経って不具合を起こすこともある、何も不具合を起こさなかったのにプログラムをどこか修正したら不具合が起こるようになることもある、という具合いで性質が悪い。

[dfbgs]

rubato6809さん回答ありがとうございます！
````
for (b = 0; b < 20; b++) {
hyo[a[b]]++;
}
````
この処理が終わった時点でbには20が代入されているということを理解していませんでした。
そしてそのa[b](a[20])はそもそも存在しない。だからバグである、と完全に理解できました😭
ありがとうございます、、、、、！
バッファオーバーランが、バグの中でも性質の悪いバグである、という周辺知識まで知ることができて、うれしいです。

一応改善したコードを載せておきます。
## コード
````
#include<stdio.h>

int main(void)
{
int a[20] = {1,2,5,2,3,4,0,1,1,5,3,4,4,2,5,4,0,1,2,3};
int b;
int hyo[6] = {0,0,0,0,0,0};
int x;
int i;

for (b = 0; b < 20; b++) {
hyo[a[b]]++;
}
printf("白票は%d 票です。\n",hyo[0]);
for (x = 1; x < 6; x++) {
printf("候補者番号%d は%d 票です。\n", x, hyo[x]);
}
int max = hyo[1];

for (i = 2; i < 6; i++) {
if (hyo[i] > max) {
max = hyo[i];
}
}
for (i = 2; i < 6; i++) {
if (hyo[i] == max) {
printf("候補者番号%d は、最大の得票数を得ました。\n", i);
}
}

return 0;
}
````
## ターミナル
````
白票は2 票です。
候補者番号1 は4 票です。
候補者番号2 は4 票です。
候補者番号3 は3 票です。
候補者番号4 は4 票です。
候補者番号5 は3 票です。
候補者番号2 は、最大の得票数を得ました。
候補者番号4 は、最大の得票数を得ました。
````

[jimbe]

折角動作確認しているのですから、結果をチェックしたほうが良いと思います。
候補番号 1 も 4 票獲得しているのに「最大の～」と表示されていません。

なお、マークダウンはコメント欄には効きません。
修正中/後のコード・実行結果は、ご質問を編集して最後に追加する形で載せては如何でしょうか。

[dfbgs]

jmbeさんご指摘ありがとうございます！
ほんとですね。見落としていました。最後のfor文の条件をi = 2 からi = 1 に変えると結果が正しくなりました。
マークダウンのご指摘ありがとうございます。
これからはマークダウンは質問に追記するようにしたいと思います。

Answer 4

switchを使おう