質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

89.13%

コードが汚すぎるので、もう少しすっきりとさせたい

解決済

回答 4

投稿

  • 評価
  • クリップ 1
  • VIEW 1,429

vnsa7221

score 306

前提・実現したいこと

現在「一週間で身につくC#言語の基本」サイトを参照しながらC#の勉強中
練習問題を一通り解き終え、Prob8のチャレンジ問題を解いており、一応希望に沿った動きを実現することができた。
ただコードが汚すぎてもう少しきれいに実装したいと考えている

対象問題
2つの分数同士の足し算をし、その結果を分数で表示するプログラムを作りなさい。このとき、分子、分母共に最大値が10で、最小値は分母が2、分子が1とする。それらの数値をランダムに発生させ、以下のように結果を表示させなさい。ただし、計算結果は、分子と分母がきちんと約分されていることとする。また、分子が分母の数で割り切れる場合は、整数として表示するものとする。


1/5 + 2/3 = 13/15     ← 通常のケース
2/3 + 3/8= 1.1/24     ← 帯分数になるケース
1/6 + 1/3= 1/2          ← 約分されるケース
1/2 + 1/2 = 1            ← 整数になるケース

発生している問題・エラーメッセージ

コードをもう少しすっきりとさせたい。主な悩みとして

  • intの連発
  • 変数大量使用
  • classも無駄な計算をしているように感じる

該当のソースコード

// program.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Problem8_6
{
    class Program
    {
        static void Main(string[] args)
        {
            @public x = new @public(); 
            Random rnd = new Random();

            int denom1 = rnd.Next(2, 11);               // 分母(1つ目の分数)
            int numer1 = rnd.Next(1, 11);               // 分子(1つ目の分数)
            int denom2 = rnd.Next(2, 11);               // 分母(2つ目の分数)
            int numer2 = rnd.Next(1, 11);               // 分子(2つ目の分数)

            Console.WriteLine("1つ目の分数→" + numer1 + " / " + denom1);
            Console.WriteLine("2つ目の分数→" + numer2 + " / " + denom2);
            Console.WriteLine();

            int gcm = x.Gcm(denom1, denom2);            // 最大公約数
            int lcm = x.Lcm(denom1, denom2, gcm);       // 最小公倍数

            Console.WriteLine("最大公約数は " + gcm + " 最小公倍数は " + lcm);
            Console.WriteLine();

            int rdNum1 = x.Fraction(denom1, numer1, lcm); // 1つ目の分数通分時の分子の値
            int rdNum2 = x.Fraction(denom2, numer2, lcm); // 2つ目の分数通分時の分子の値
            int rdSum = rdNum1 + rdNum2;

            Console.WriteLine("分母:" + lcm + " 分子1:" + rdNum1 + " 分子2:" + rdNum2 + " 分子1 + 分子2:" + rdSum);
            Console.WriteLine();

            string ans = x.Reduction(lcm, rdSum);

            Console.WriteLine("計算値は " + ans);
        }
    }
}
// public.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Problem8_6
{
    class @public
    {
        // 最大公約数を表示
        public int Gcm(int d1, int d2)
        {
            if(d1 < d2)
            {
                return Gcm(d2, d1);
            }

            while(d2 != 0)
            {
                var remain = d1 % d2;     // 余り
                d1 = d2;                  // 次の計算時にbの値をaに設定
                d2 = remain;              // 次の計算時にremain(余り)の値をbに設定
            }

            return d1;
        }

        // 最小公倍数を表示
        public int Lcm(int d1, int d2, int gcm)
        {
            return d1 * d2 / gcm;
        }

        // 通分
        public int Fraction(int den, int num, int lcm)
        {
            return lcm / den * num;      // 分母にて、通分するために倍数計算し、分子に反映
        }

        // 約分
        public string Reduction(int lcm, int rdsum)
        {
            int tmp = rdsum / lcm;      // 分子合計 / 分母通分値で帯分数の整数を算出
            int ans = rdsum % lcm;      // 分子合計 / 分母通分地で帯分数の分子を算出

            if (ans == 0)
            {
                return tmp.ToString();  // 整数値を文字列化して返却
            }
            else if(tmp >= 1)
            {
                if(Gcm(ans, lcm) != 1)
                {
                    int gcm = Gcm(ans, lcm);    // 分子と分母の最大公約数を算出
                    ans /= gcm;                 // 分子を最大公約数で割る
                    lcm /= gcm;                 // 分母を最大公約数で割る
                }
                return tmp.ToString() + " . " + ans.ToString() + " / " + lcm.ToString();
            }
            else
            {
                if (Gcm(lcm, rdsum) != 1)
                {
                    int gcm = Gcm(lcm, rdsum);    // 分子と分母の最大公約数を算出
                    lcm /= gcm;                   // 分母を最大公約数で割る
                    rdsum /= gcm;                 // 分子を最大公約数で割る
                }
                return rdsum.ToString() + " / " + lcm.ToString();
            }
        }
    }
}

補足情報(言語/FW/ツール等のバージョンなど)

C#を触り始めてちょうど1週間くらいになる初心者です。
この部分の処理はこうすればもっと少ない量で書けます。やこれを追加すればこんな面倒なことする必要ない等アドバイスを頂けると非常に幸いです。
よろしくお願いいたします。

  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

質問への追記・修正、ベストアンサー選択の依頼

  • mattn

    2017/06/28 09:12

    念のため言っておきたいですが、コードそんなに汚くないですよ(public以外)

    キャンセル

回答 4

checkベストアンサー

0

今時は、文字列連結は

Console.WriteLine($"1つ目の分数→{numer1}/{denom1}");


が便利ですね。
{}で囲まれたところで計算してもいいので、それだけで見通しは良くなりますね。

Console.WriteLine($"最大公約数は {x.Gcm(denom1, denom2)} 最小公倍数は {x.Lcm(denom1, denom2, gcm)}");


こんな感じ。

class名 publicはやらないほうがいいかと。@は気持ち悪い。こういう特殊な記法は、絶対その言葉を使わないとだめなときだけです。今回は絶対使わないとだめなときではありません。

あと

  public int Lcm(int d1, int d2, int gcm)
        {
            return d1 * d2 / gcm;
        }


  public int Lcm(int d1, int d2, int gcm)=> d1 * d2 / gcm;


こう書けますね。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/06/28 14:39

    kiichi54321さん回答ありがとうございます。
    文字列連結やclass名に関して等様々なアドバイスをしていただきありがとうございます。
    class名の部分は何も考えずに入れてましたが、問題があるということを知れたのでとてもよかったです。
    いただいたアドバイスを参照し、コードの修正を加えていこうと思います。
    ご丁寧なアドバイスをいただきありがとうございました。

    キャンセル

0

kiichi54321 さんが既に書かれているので、それ以外の部分で。
不要なインポートがあるので消した方が良いかもしれません。見たところ System だけで行けそう。

あと Reduction の tmp は計算を遅らせる事は出来そう。(大した話ではないですが)

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/06/28 14:42

    mattnさん回答ありがとうございます。
    私の知識不足もあり、不要なインポートって何だろうと思ってしまいました・・・。
    この辺りはググりながら勉強していくしかないですね。
    また、そこまで汚くないなどフォローしていただきありがとうございます。
    もっと他の人が見ただけでわかるようなコードにしていきたいです。
    いただいたアドバイスを参照し、コードの修正を加えていこうと思います。
    ご丁寧なアドバイスをいただきありがとうございました。

    キャンセル

0

そんなに悪くないと思います。

一番、気持ち悪いのが@publicですかね。なぜ、そんな名前を・・・

intの連発
変数大量使用

publicは関数クラス(計算専用)なので、メソッドのたびに変数を与える必要があるので、
呼び出し側(Program)は、どうしてもint変数が必要になってしまいます。

対処方法は、publicの内部に変数を持つことで、コンストラクタで1回渡すようにすれば、
Programからはintを追い出すことができます。

 Console.WriteLine("最大公約数は " + gcm + " 最小公倍数は " + lcm);
// ↓のように、直接メソッドを呼び出せるので中間の変数(lcm、gcm)が不要
 Console.WriteLine("最大公約数は " + x.gcm() + " 最小公倍数は " + x.lcm());

publicは、計算処理だけにしたくて変数持ちたくない!というポリシーあるならば、
別途、publicを内部にもつクラスを新規に作成すればよいと思います。

classも無駄な計算をしているように感じる

変数持つようにすれば、最大公約数の計算も1回ですみます。

あとは、使い勝手として
lcmや、rdSumを外部から与えないと、Fraction、Reductionが呼べない

int rdNum1 = x.Fraction(denom1, numer1, lcm); // 1つ目の分数通分時の分子の値
int rdSum = rdNum1 + rdNum2;

string ans = x.Reduction(lcm, rdSum);

というのは不便なので、やはり変数持つのをオススメします。
rdSumやlcmを内部でもてば、Programから渡す必要ないです。

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/06/28 14:45

    momon-gaさん回答ありがとうございます。
    class名や変数削除等様々なアドバイスをしていただきありがとうございます。
    内部で変数を持って置き、外部からいちいちパラメータを与えずに処理ができるようにするという手法に関しては、これから勉強していきたいと思います。
    いただいたアドバイスを参照し、コードの修正を加えていこうと思います。
    ご丁寧なアドバイスをいただきありがとうございました。

    キャンセル

0

  • コードをみたらわかることにコメントいらない
  • 命名のセンス(publicとかtmpとかansとか)
  • コードブロックはできるだけ小さく

あたりを気を付けたらよいかと

ifブロックはできるだけ小さくを心がけないと、実物のコード書き出したら追いきれなくなります

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/06/28 14:48

    dojikkoさん回答ありがとうございます。
    コメントや命名センス等様々なアドバイスをしていただきありがとうございます。
    今後実務で使用していくにあたりifブロックのコーディングは気を付けていこうと思います。
    いただいたアドバイスを参照し、コードの修正を加えていこうと思います。
    ご丁寧なアドバイスをいただきありがとうございました。

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 89.13%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる