C言語です。採点お願いします。

Question

C言語で2から1000の中の素数を求めるプログラムを作ってみました。流石にわからなすぎてインターネットからヒントだけもらって作りました！！
採点お願いします。
もっと読みやすくカッコ良く書く方法があれば是非教えてほしいです！
問題は独習Cの練習問題3.6の大問2です。

#include<stdio.h>
int main(void)
{
　　　　int suuji,waru,df = 0,watta = 0;
　　　　for (suuji = 2; suuji < 1001; suuji++) {
　　　　　　　　df = 0;
　　　　　　　　for(waru = 1; waru <= suuji; waru++) {
　　　　　　　　　　　　watta = suuji% waru;
　　　　　　　　　　　　if (watta == 0) {
　　　　　　　　　　　　　　　　df++;
　　　　　　　　　　　　}
　　　　　　　　}
　　　　　　　　if(df == 2) {
　　　　　　　　　　　　printf("%d",suuji);
　　　　　　　　}
　　　　　　　　printf(" ");
　　　　}
　　　　return 0;
}

Comments

[Zuishin]

インデントが酷すぎてそれ以上読む気になりません。

[tyaaaaaaaaaaras]

すいません。直します

[Zuishin]

あと画像は論外です。普通コードを読むときには目で読むのではなくツールを使いますが、画像ではそれができません。

[tyaaaaaaaaaaras]

わかりました。
ごめんなさい。言い訳に思えてしまうでしょうが本当に初心者でわかりませんでした。修正します

[Zuishin]

質問文はマークダウンを用いて書くようになっています。

```C
int main(void) {
}
```

このように、```C と ``` を単独行にして、その間にコードを書いてください。

[Zuishin]

確認しました。修正お疲れ様でした。

[tyaaaaaaaaaaras]

ありがとうございます。修正してみました！よろしくおねがいします。

[退会済みユーザー]

回答ではないですが、この記事を紹介しておきます
https://qiita.com/alt_yamamoto/items/25eda376e6b947208996

[miyabi_pudding]

スパゲティコードの意味を勉強した方がいいかと思いますよ。
この、一ファイルで完結する一処理である以上は、スパゲティコードになりようがないかと。

[SaitoAtsushi]

この質問は事実上のコードレビュー依頼です。
その手の漠然とした質問は Teratail の主旨に反しています。 (ただちに規約違反というわけではありませんが。)
https://teratail.com/help/avoid-asking
また、プログラムが仕様に沿うかどうかならともかく、作法に適うかというのはそのプログラムがどのような運用を意図しているかによっても左右されるため、運用が存在しない演習の類について指摘しようとすると回答も漠然としたものにならざるを得ません。
想定する運用を仮にでも設定するなど、質問をより具体的にしてください。

Answer 1

個人的に見づらい・わかりづらい表現だなと思うところのみ指摘します。

■1行に詰め込まない

今回のコードでは変数宣言のみですが、1行には詰め込まないほうが見やすいと思います。
（関連性のある変数を意図的にまとめた方が見やすい場合もあるので絶対ではありません）

- int suuji,waru,df = 0,watta = 0;
+ int suuji;
+ int waru;
+ int df = 0;
+ int watta = 0;

■for文のループ条件

仕様で「2から1000の中の素数」と書かれているので、1001と書くのではなく1000と書いた方がわかりやすいです。

- for (suuji = 2; suuji < 1001; suuji++) {
+ for (suuji = 2; suuji <= 1000; suuji++) {

■文字間・行間などメリハリをつける

変数宣言と処理の間に空行を入れる。

  int suuji,waru,df = 0,watta = 0;
+
  for (suuji = 2; suuji < 1001; suuji++) {

打ちミスかもしれませんが、詰めると見づらいです。

- watta = suuji% waru;
+ watta = suuji % waru;

同様。

- printf("%d",suuji);
+ printf("%d", suuji);

■余りを変数に受け取る必要なし

wattaをあとで他の用途で使うなら別ですが、今回の処理では不要です。

- watta = suuji% waru;
- if (watta == 0) {
+ if (suuji% waru == 0) {
      df++;
 }

■不要な初期化

dfはループ内先頭で初期化している、wattaは計算結果を受け取るのみなので初期化は不要です。

- int suuji,waru,df = 0,watta = 0;
+ int suuji,waru,df,watta;

■4タブにする

8タブになっているのはこの質問上だけでしょうか？
ネストが深くなるにつれ見づらくなるので、4タブにしたほうがよいでしょう。

色々指摘しようかと思いましたが、1関数しかない数行のコードでは大して指摘もありませんでした。
何かを参考にしたようですので、取り立てて無駄なところもないように見えます。
素数のアルゴリズム自体については賛否両論あるでしょうけど、本質問とは関係ないと思うので割愛します。

以下、修正後ソース。

#include<stdio.h>

int main(void)
{
	int suuji;
	int waru;
	int df;

	for (suuji = 2; suuji <= 1000; suuji++) {
		df = 0;
		for (waru = 1; waru <= suuji; waru++) {
			if (suuji % waru == 0) {
				df++;
			}
		}
		if (df == 2) {
			printf("%d", suuji);
		}
		printf(" ");
	}
	return 0;
}

Answer 2

スパゲティコードではないと思います（というか、単純なコードなのでスパゲティにならない）
わかりやすいか？と聞かれたら微妙かなぁと思います。

2から1000の中の素数を表示する

これをそのままコードとして表現したほうが、わかりやすいです。
fanaさんの回答の通り関数化すると、コードの詳細をみなくても何をしてるかすぐイメージできます。
※C言語なれてないので、おかしいかもしれないです

// ２～１０００の中の素数を表示
for (int number = 2; number <= 1000; number++) {
	if (is_prime(number)) {
		printf("%d ", number);
	}
}

関数を使わないなら変数の命名で代用する

for (int number = 2; number <= 1000; number++) {
	int isPrime = 1;
	for (int i = 2; i < number; i++) { // １と自身以外で以下判定
		if (number % i == 0) { // 割り切れる数字があれば素数でない
			isPrime = 0;
			break;
		}
	}
	if (isPrime == 1) {
		printf("%d ", number);
	}
}

Comments

[退会済みユーザー]

変数の宣言もこの書き方が好みですね。昔のC言語はブロックの先頭で変数宣言する必要があったのですが、今時のコンパイラはそういう制限は無いので、まとめて変数を宣言するより、変数を使用する箇所でその都度宣言し、変数の有効なスコープを可能な限り短くしたい所です。（コンパイラが対応してない場合は仕方ないのですが）

Answer 3

カッコ良く

とは何か？　主観的で不明瞭な話だが……
少なくとも私が「かっこ悪い」と思う点があるので挙げてみる．
（i.e. それを正せば少しはカッコ良くなる？）

• インデントがぐちゃぐちゃ．
• 日本語的な変数名が個人的にダサいと思う．あと，dfはどういう意味で付けたのかもわからない．
• ある値を「1とその値自身とで割り切れること」は計算するまでもなく自明である．それをわざわざ計算してカウントしているのがダサい

読みやすく

に関しては，「ある値が素数であるか否かを判定する処理」を（そういう処理だとわかる名前の）関数にでもしてやればどうでしょう．

Answer 4

素数の定義：
「素数（そすう、英: prime number）とは、1 より大きい自然数で、正の約数が 1 と自分自身のみであるもののことである。」
に従えば、あなたのプログラムは、全くその通りで、100点満点です。

実際に実行したときには、いくつかの改善すべき点が見られます。

１．出力結果について

上記のようになり、みにくくなっています。
（１）各素数間の空白が一定でない。
（２）どこかで改行があれば見やすい。

２．fanaさんも言ってますが、１で割ると割り切れるのは自明なので、それは不要です。
処理速度等を考慮すると、
（１）１で割るのは、やめて、２から開始する。
（２）割り切れた回数を数えるのではなく、割り切れたとき、waruの値とsuujiを比較し、
それが、同じでないなら、自分自身で割ったのではないので、素数ではない、と判断すべき。

上記を考慮して修正した結果です。

#include<stdio.h>
int main(void)
{
        int suuji,waru,df = 0,watta = 0;
        int flag,counter = 0;
        for (suuji = 2; suuji < 1001; suuji++) {
                flag = 0;
                for(waru = 2; waru <= suuji; waru++) {
                        watta = suuji% waru;
                        if (watta == 0) {
                                if (waru == suuji){
                                    flag = 1;
                                    counter++;
                                }
                                break;
                        }
                }
                if(flag == 1) {
                        printf("%5d",suuji);
                        if (counter % 10 == 0){
                                printf("\n");
                        }
                }
        }
        return 0;
}

実行結果