3
\$\begingroup\$

My program will divide the decimal (\$30.0575\$) by a range of fractions (\$\frac{1}{1}, \frac{1}{2}, \frac{1}{4}, \dots, \frac{1}{48})\$ leaving me with list of improper fractions that look like this:

  • \$\frac{1}{1} * 30.0575\$
  • \$\frac{1}{2} * 60.115\$
  • \$\frac{1}{3} * 90.1725\$
  • \$\dots\$
  • \$\frac{1}{48} * 1442.76\$

My program will then "pick out" the improper fraction that contains the decimal that is closest to a whole number. It will finally convert the improper fraction that is closest to a whole number into a mixed fraction.

  1. How can I make my program more readable?
  2. Are there any "shortcuts" I could implement that would reduce the lines of code used?
  3. How can I overall optimize my code so it completes the same task in a more efficient way, while still keeping readability?

private void FractionsForm_Load(object sender, EventArgs e)
        {
            var myDecimal = 30.0575;

            var results = new List<double>();

            for (double i = 1; i < 49; i++)
            {
                var fraction = 1 / i;
                var result = myDecimal / fraction;
                results.Add(result);
                Console.WriteLine("1/{0} * {1}", i, result);
            }

            WriteLineSeparator();

            var remainders = new List<double>();

            foreach (var result in results)
            {
                var roundedResult = Math.Round(result);
                var remainder = roundedResult - result;
                remainders.Add(Math.Abs(remainder));
            }

            var mostAccurateRemainder = remainders.Min();
            var mostAccurateRemainderIndex = remainders.IndexOf(mostAccurateRemainder);
            var fractionDenominator = mostAccurateRemainderIndex + 1;

            Console.WriteLine("1/{0} * {1}", fractionDenominator, results[mostAccurateRemainderIndex]);

            WriteLineSeparator();

            var integerPart = Convert.ToInt32(myDecimal);
            var integralPart = myDecimal - Math.Truncate(myDecimal);

            Console.WriteLine("{0} {1}/{2}", integerPart, Convert.ToInt32(integralPart * fractionDenominator),
                fractionDenominator);
        }

        private static void WriteLineSeparator()
        {
            Console.WriteLine(string.Empty);
            Console.WriteLine("--------------------------------------------------");
            Console.WriteLine(string.Empty);
        }

        /*

            1/1 * 30.0575
            1/2 * 60.115
            1/3 * 90.1725
            1/4 * 120.23
            1/5 * 150.2875
            1/6 * 180.345
            1/7 * 210.4025
            1/8 * 240.46
            1/9 * 270.5175
            1/10 * 300.575
            1/11 * 330.6325
            1/12 * 360.69
            1/13 * 390.7475
            1/14 * 420.805
            1/15 * 450.8625
            1/16 * 480.92
            1/17 * 510.9775
            1/18 * 541.035
            1/19 * 571.0925
            1/20 * 601.15
            1/21 * 631.2075
            1/22 * 661.265
            1/23 * 691.3225
            1/24 * 721.38
            1/25 * 751.4375
            1/26 * 781.495
            1/27 * 811.5525
            1/28 * 841.61
            1/29 * 871.6675
            1/30 * 901.725
            1/31 * 931.7825
            1/32 * 961.84
            1/33 * 991.8975
            1/34 * 1021.955
            1/35 * 1052.0125
            1/36 * 1082.07
            1/37 * 1112.1275
            1/38 * 1142.185
            1/39 * 1172.2425
            1/40 * 1202.3
            1/41 * 1232.3575
            1/42 * 1262.415
            1/43 * 1292.4725
            1/44 * 1322.53
            1/45 * 1352.5875
            1/46 * 1382.645
            1/47 * 1412.7025
            1/48 * 1442.76

            --------------------------------------------------

            1/35 * 1052.0125

            --------------------------------------------------

            30 2/35

        */
\$\endgroup\$
  • 1
    \$\begingroup\$ It's kind of weird that you are doing your processing in a form load and using console. Writeline for output . Wouldn't a console app have been more appropriate? \$\endgroup\$ – forsvarir Jul 9 '16 at 17:23
  • \$\begingroup\$ @forsvarir It would be. However, I'm going to make the program interact with text box controls later on. :) \$\endgroup\$ – Owen Jul 9 '16 at 17:36
  • \$\begingroup\$ If your intention is to find fractions which are good approximations to a given floating point number then you should have a look at continued fractions. \$\endgroup\$ – Martin R Jul 9 '16 at 20:51
4
\$\begingroup\$
  • Trust the math.

            var fraction = 1 / i;
            var result = myDecimal / fraction;
    

    is a long way to say

            var result = myDecimal * i;
    
  • Collecting results in the list is just a waste of space. You may maintain the most accurate candidate as you do compute results:

        for (double i = 1; i < 49; i++)
        {
            var result = myDecimal * i;
            var roundedResult = Math.Round(result);
            var remainder = roundedResult - result;
            if (remainder < bestRemainder) {
                bestRemainder = remainder;
                bestDenominator = i;
            }
        }
    
\$\endgroup\$
  • \$\begingroup\$ I guess the OP made a mistake in his code because he writes a multiplication in all his examples. \$\endgroup\$ – t3chb0t Jul 9 '16 at 19:10
4
\$\begingroup\$

To test your code I removed the references to the Event handler, and any WinForms references, because they'r not used in this snippet and irrelevant to your question.

I noticed a few weird things, you have this code:

var fraction = 1 / i;
var result = myDecimal / fraction;

And other than the loss of precision inherent in floating numbers, there is no difference between this and myDecimal * i. And your output ("1/{0} * {1}", i, result) shows the fraction being multiplied by the result, so unless you have a specific reason for formatting it that way, I assume using a comma would be more readable. Oh and use string interpolation : $"1/{i} , {result}"

For calculating the reminders this way is also a bit ... weird. Keep in mind that Math.Round does not work like you'd expect so you don't get the most predictable rounding. May I suggest the following:

var candidate = result % 1;
var remainder = Math.Min(candidate , 1 - candidate);

Using the modulus operator % like that rids of the integer part and keeps the fractional part. You can use this trick later in your code, too. Consider using Math.Floor to extract the integer part.

Last but not least, use LINQ !! And look at Functional programming techniques, as they are perfect for this sort of problems. Here is your main logic written, perhaps amateurishly, in LINQ queries: you'll wonder how you ever lived without them. You can add printing and processing the results later, I guess.

var myDecimal = 30.0575;

var bestestFraction =
    Enumerable.Range(1, 48)
              .Select(x => x * myDecimal)
              .OrderBy(x => Math.Min(x % 1, 1 - (x % 1)))
              .Select(x => x / myDecimal)
              .First();

If you have Visual Studio 2015 Update 2.0, check this out in the C# interactive. It will give you 35.0 as the answer. do with that as you will.

\$\endgroup\$
2
\$\begingroup\$

You've already had some feedback on your algorithm, so I'm not going to touch that, I'm just going to look at the code.

Be Explicit

I found it weird that you'd written your function within the FractionsForm_Load method. You've explained that you're going to add controls to the form later that the code may interact with. Using the form to test your code is fine, but be explicit about what it is you're actually testing. This will also help you to get the most out of CodeReview.

As it stands, your function does several things and it's not really clear how important each step is. It could be that all of your Console.WriteLine statements are simply for debug purposes and what you really care about is the closest mixed number. If this is the case, then I would actually wrap all of the code in a mixed number class, something like this:

class MixedNumber
{
    public MixedNumber(double source) { ... }

    public int WholeNumber {  get { ... } }
    public int Numerator { get { ... } }
    public int Denominator { get { ... } }
}

This could then be used from your form. Notice this is class takes the initial number as an argument, because it seems unlikely that you're only really interested in the number 30.0575.

Of course, it may be that all of those Console.WriteLine statements are important and you're planning on replacing them with writing to a text area or something. If that's the case, I'd consider replacing Console.WriteLine with statements that write to a TextWriter. You could then pass in Console.Out initially and easily replace it by something else in the future.

Either way, I would define a function and call it from the Form_Load, not just write the code into the form load. This will also make it easier to reuse the functionality if you want to trigger a recalculation when an input number box changes or a submit button is pressed etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.