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

Question

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

対象問題

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

例

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週間くらいになる初心者です。
この部分の処理はこうすればもっと少ない量で書けます。やこれを追加すればこんな面倒なことする必要ない等アドバイスを頂けると非常に幸いです。
よろしくお願いいたします。

Comments

[mattn]

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

Answer 1

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

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

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

Comments

[vnsa7221]

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

Answer 2

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

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

intの連発

変数大量使用

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

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

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

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

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

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

あとは、使い勝手として
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から渡す必要ないです。

Comments

[vnsa7221]

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

Answer 3

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

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

Comments

[vnsa7221]

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

Answer 4

今時は、文字列連結は

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;

こう書けますね。

Comments

[vnsa7221]

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