C言語 プログラムのチェックと質問

質問

プログラミング初心者です
プログラミングの練習のためにプログラムを組みました
ですがあまり綺麗なプログラムにならなかったのでもっとこうしたらいい
というところがあれば教えていただけないでしょうか
また組んでいて改善したいところがあったのですが
自分ではどうしたらいいかわからなかったので教えていただけませんか

プログラムの説明
1から４３の数字からランダムに6つ選び小さい順に並べる
それを５回繰り返すプログラム
つまりロト６のクイックピックのようなことをするプログラムです

改善したい点
異なる行で2つ以上同じ数がないようにしたい

以下ソースコード

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

int main(){
int sai[10][10],i1,i2,i3,j1,j2,j3,num[50],temp;

srand((unsigned)time(NULL));

for(i1=0; i1 < 6; i1++){
for(i2= 1; i2 < 44; i2++){
num[i2] = i2;
}

for(i3 = 0; i3 < 6 ; i3++){
sai[i3][i1] = rand() % 43 + 1;

while( num[sai[i3][i1]] == 0){

sai[i3][i1] = rand() % 43 + 1;
}
num[sai[i3][i1]] = 0;

}

}

for(j1 = 0; j1 < 6 ; j1++){
for(j2 = 0; j2 < 10 ; j2++){
for(j3 = 0; j3 < 5 ; j3++){
if(sai[j3][j1] > sai[j3 + 1][j1]){
temp = sai[j3][j1];
sai[j3][j1] = sai[j3 + 1][j1];
sai[j3 + 1][j1] = temp;
}
}

}
}

for(j1 = 0; j1 < 5 ; j1++){
for(j2 = 0; j2 < 6 ; j2++){
printf("%d ",sai[j2][j1]);
}
printf("\n");
}

return 0;
}

以上

回答No.2

yama5140 · ベストアンサー

＞もっとこうしたらいいというところがあれば教えていただけないでしょうか

今回、二次元配列は、「６数字」「５組」として使われています。
「５組」「６数字」とするのが、違和感？がないです。
（これが私のプログラミングスタイル、といわれればそれまでですが・・）

また、sai[10][10] などと多めに確保するのは、エラーを「出さない」ためにはいいかもしれませんが、「エラー」が隠れてしまうことになりかねません。
今回でも、sai[j2][j1] を sai[j1][j2] とタイプミスしてもエラーはでませんが、思ったとおりに動かない。
こういったミスはなかなか気づかず、アルゴリズムが悪いのか、となってしまう。
キチッと配列を確保しておくと、実行時にタイプミスに気づきます。

配列を多めに確保して幸せになれるのは、「文字列」を扱うときかと・・。

＞もっとこうしたらいいというところがあれば教えていただけないでしょうか

「関数」を利用したらどうでしょう。

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

void　Make1set( int i1Set[] )
{
　int　i, j, iNum, iTemp, iMin;
　int　iUse[ 44 ];　// 重複使用確認用

　for( j = 1; j <= 43; j++ )　iUse[ j ] = 0;　// 初期化

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

　　while( 1 ){

　　　iNum = rand() % 43 + 1;

　　　if( 0 == iUse[ iNum ] ){

　　　　iUse[ iNum ]++;
　　　　break;
　　　}
　　}
　　i1Set[ j ] = iNum;
　}
　for( i = 0; i < 5; i++ ){　// 昇順並び替え

　　iMin = i1Set[ i ];

　　for( j = i; j < 6; j++ ){

　　　if( iMin < i1Set[ j ] )　continue;

　　　iMin = i1Set[ j ];
　　　i1Set[ j ] = i1Set[ i ];
　　　i1Set[ i ] = iMin;
　　}
　}
}
void　Output( int i1Set[] )
{
　int　i;

　for( i = 0; i < 6 ; i++ ){

　　printf( "%2d ", i1Set[ i ] );
　}
　printf( "\n" );
}
int　Check2EQ( int iNarabi[ 5 ][ 6 ], int iii )
{
　int　i, j, k, iEQCnt;

　for( i = 0; i < iii; i++ ){

　　iEQCnt = 0;

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

　　　for( k = 0; k < 6; k++ ){

　　　　if( iNarabi[ iii ][ j ] == iNarabi[ i ][ k ] )　iEQCnt++;
　　　}
　　}
　　if( 2 <= iEQCnt ){　// 異なる組で２つ以上同じ数

　　　printf( "retry " );
　　　Output( iNarabi[ iii ] );

　　　return( 1 );
　　}
　}
　return( 0 );
}
int　main()
{
　int　iNarabi[ 5 ][ 6 ];　// ６ケの数字５組
　int　i, iEQ2;

　srand( (unsigned)time(NULL) );

　for( i = 0; i < 5; i++ ){　// ５回繰り返す

　　do{
　　　Make1set( iNarabi[ i ] );
　　　iEQ2 = Check2EQ( iNarabi, i );

　　}while( iEQ2 );　// 異なる組で２つ以上同じ数がなくなるまで

　　Output( iNarabi[ i ] );
　}
　return( 0 );
}
注：インデントに全角空白を用いています。コピペ後、タブに一括変換して下さい。

お礼

回答ありがとうございます
今後は配列で無駄を作らないことと関数を利用することにも
気をつけてみようと思います
ありがとうございました

回答No.4

maru_yoshi_

> あまり綺麗なプログラムにならなかったのでもっとこうしたらいい
> というところがあれば教えていただけないでしょうか
では、他の人が指摘していない観点からアドバイスを一つ。

コメントをつけるようにしましょう。
コードを書いているときには分かっていることでも時間がたつとそのコードの意味を忘れてしまいます。
/* 一週間後なら覚えているが、一ヵ月後は？、１年後は？ */
コメントがあるとそのコードを見た人の理解が早まり、アドバイスをもらいやすくなります。
もっというと、自分でコードの意味を考えることにより、整理することができ、自分で間違いを見つけることが出来るかも知れません。

かといって、多すぎるコメントは別の意味で問題なんですけどね。
上の回答とはちょっと矛盾しているけど、若い人にはコメントなしで読めるソフトを書け！　と言っている私。

お礼

回答ありがとうございます
確かにほかの方の回答のソースコードも
コメントがあったのでわかりやすかったです
今後はコメントをつけるようにしてみます
ありがとうございました

回答No.3

hitomura

なんかプログラム作っているうちにほかの人の回答が来ちゃったなぁ…
とりあえずyama5140さんがしていないアドバイスとしては、定数は#defineで定義しましょう、ということでしょうか。

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

#define SELECT_FROM 43
#define NUM_SELECT_NUMBER 6
#define NUM_SELECTION 5
#define ACCEPTABLE_MATCH_MAX 1

/*
* 番号選択
*/
void select(int result[])
{
　int table[SELECT_FROM];
　int i, j;

　memset(table, 0, sizeof(int) * SELECT_FROM);

　for (i = 0; i < NUM_SELECT_NUMBER; i++){
　　int selected = rand() % (SELECT_FROM - i);
　　for (j = 0; j < SELECT_FROM; j++){
　　　if (table[j]) selected++;
　　　if (j == selected){
　　　table[j] = 1;
　　　break;
　　　}
　　}
　}

　i = 0;
　for (j = 0; j < SELECT_FROM; j++){
　　if (table[j]){
　　　result[i] = j + 1;
　　　i++;
　　}
　　if (i == NUM_SELECT_NUMBER) break;
　}
}

/*
* 2つの選択した数列で同一の数字の個数を数える
*/
int count_matched(int a1[], int a2[])
{
　int i1 = 0, i2 = 0;
　int matched = 0;
　while ((i1 < NUM_SELECT_NUMBER) && (i2 < NUM_SELECT_NUMBER)){
　　if (a1[i1] == a2[i2]){
　　　matched++;
　　　i1++;
　　　i2++;
　　} else if (a1[i1] < a2[i2]){
　　　i1++;
　　} else { /* ここにくるのは a1[i1] > a2[i2] のとき */
　　　i2++;
　　}
　}

　return matched;
}

int main()
{
　int sai[NUM_SELECTION][NUM_SELECT_NUMBER],i1,i2,j1,j2;

　srand((unsigned)time(NULL));

　for(i1=0; i1 < NUM_SELECTION; i1++){
　　int matched_twice;
　　do {
　　　matched_twice = 0;
　　　select(sai[i1]);
　　　for(i2 = 0; i2 < i1 ; i2++){
　　　　if (ACCEPTABLE_MATCH_MAX < count_matched(sai[i1], sai[i2])){
　　　　　matched_twice = 1;
　　　　　break;
　　　　}
　　　}
　　} while (matched_twice);
　}

　for(j1 = 0; j1 < NUM_SELECTION; j1++){
　　for(j2 = 0; j2 < NUM_SELECT_NUMBER; j2++){
　　　printf("%d ",sai[j1][j2]);
　　}
　　printf("\n");
　}

　return 0;
}

なお、インデントに全角空白を用いています。コピペ後、タブに一括変換して下さい。

……たぶんほかの回答者に突っ込まれるだろうからその前に弁明。
配列を関数に渡す場合はその要素数を一緒に渡すか、いわゆる番兵と呼ばれるデータを設定して渡すのが常道です。しかし、上記のプログラムでは今言った手法のいずれも使用しておりません。
これは、今回の場合はそれぞれの扱う配列の要素数が固定で、その要素数はすべて#define定義の値を使用しているため変更があったとしても一括で行えること、また、ルーチンの呼び出し元が1つでほかへの流用が（今のところ）ないため問題ないだろうという判断です。
要するに、将来XX個の中からY個とってくるプログラム（XX,Yは利用者が指定）への修正がないだろうという判断です。

お礼

回答ありがとうございます
#defineによる定義についても勉強しようと思います
非常にわかりやすかったです
ありがとうございました

回答No.1

yaemon_2006

こんな感じかな？

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

void init(int num[], int n)
{
int i;

for(i = 0; i < n; ++ i) num[i] = i + 1;
}

void shuffle(int num[], int n)
{
int i;

for(i = n - 1; i; -- i){
int j = rand() % (i + 1), k = num[i];

num[i] = num[j];
num[j] = k;
}
}

void sort(int a[], int n)
{
int i;

for(i = n - 1; i; -- i){
int j;

for(j = 0; j < i; ++ j){
if(a[j] > a[j + 1]){
int k = a[j];

a[j] = a[j + 1];
a[j + 1] = k;
}
}
}
}

void print(const int a[], int n)
{
int i;

for(i = 0; i < n; ++ i) printf("%2d ", a[i]);
putchar('\n');
}

int main(void)
{
int num[43], i;

init(num, 43);
srand((unsigned)time(NULL));
shuffle(num, 43);
for(i = 0; i < 5; ++ i) sort(&num[i * 6], 6);
for(i = 0; i < 5; ++ i) print(&num[i * 6], 6);
return 0;
}

お礼

回答ありがとうございます
すごくわかりやすいプログラムでした
こんなプログラムが組めるように頑張りたいと思います
ありがとうございました