3
\$\begingroup\$

I got this exercise on a job interview and i'd like to know how can i improve my code.

Exercise:

Create a function that returns a reduced version of a fraction.

Examples

  • Reduce("4/6") = "2/3"
  • Reduce("10/11) = "10/11"
  • Reduce("100/400") = "1/4";

Notes: a reduced fraction doesn't have a lowest common divisor (except 1) between its numerator and denominator. For example, 4/6 it's not reduced, given that 4 and 6 share 2 as a factor. If a fraction can be converted to a whole number, it also has to be considered.

My code:

    class Program
    {
        static void Main(string[] args)
        {
            string[] fractions =
            {
                "-24/12",
                "-3/2",
                "3/-1",
                "-6/-2",
                "3/0",
                "3/8/1",
                "4a/6",
                "2/8a",
                "////"
            };

            foreach (var frac in fractions)
            {
                Console.WriteLine($"{Reduce(frac)}");
            }

            Console.ReadLine();
        }

        static string Reduce(string fraction)
        {
            string[] members = fraction.Split('/');

            // Verify that the fraction received only has two numbers separated by '/' and that its denominator is not 0.
            if (members.Length != 2 || !int.TryParse(members[0], out int numerator) || !int.TryParse(members[1], out int denominator) || denominator == 0)
            {
                return $"Fraction's {fraction} format is incorrect";
            }

            // Check if the received fraction is negative.
            var isNegative = (numerator >= 0 ^ denominator >= 0);

            // Find the greatest common denominator and the reduce.
            numerator = Math.Abs(numerator);
            denominator = Math.Abs(denominator);

            var gcd = GCD(numerator, denominator);

            numerator /= gcd;
            denominator /= gcd;

            // If the fraction is negative, set the numerator to negative.
            if (isNegative) numerator *= -1;

            // Return the reduced fraction
            if (denominator == 1) return numerator.ToString();
            return $"{numerator}/{denominator}";
        }

        // Calculates the greatest common denominator between two positive integers.
        static int GCD(int num, int den)
        {
            int gcd = 1;

            for (int i = 2; i <= num && i <= den; i++)
            {
                if (num % i == 0 && den % i == 0)
                {
                    gcd = i;
                }
            }

            return gcd;
        }
    }

\$\endgroup\$
1
  • \$\begingroup\$ What does your code output for Reduce("4/2"), Reduce("4/-6"), Reduce("-6/-2"), or any ill formated fractions involving 0 or infinity? \$\endgroup\$ Jun 15, 2021 at 22:54

2 Answers 2

2
\$\begingroup\$

GCD

private int CalculateGreatestCommonDenominator(int numerator, int denominator)
{
    int? greatestCommonDenominator = null;

    for (int divisor = 2; divisor <= numerator && divisor <= denominator; divisor++)
        if (numerator % divisor == 0 && denominator % divisor == 0)
            greatestCommonDenominator = divisor;

    return greatestCommonDenominator ?? 1;
}
  • I would suggest trying to use meaningful / self-expressive names
    • If you are using expressive naming then you can get rid of the "explanation comments"
  • I always recommend to try to capture the Why and Why not decisions in the comments. Not the What and/or How
    • The latter ones should be told by the code itself
    • The formers ones are those that are not obvious from the code
      • for example Why did I choose algorithm X instead of algorithm Y?
  • I would also suggest to distinguish default and fallback values
    • In this case the 1 is the fallback value that's why in my opinion it is better to use that as a fallback

Reduce

public (int dividend, int? divisor) Reduce(string fraction)
{
    string[] members = fraction.Split('/');

    if (members.Length != 2
        || !int.TryParse(members[0], out int numerator)
        || !int.TryParse(members[1], out int denominator)
        || denominator == 0)
    {
        throw new InvalidOperationException($"The following fraction '{fraction}' has an incorrect format.");
    }

    var isNegative = numerator >= 0 ^ denominator >= 0;

    var positiveNumerator = Math.Abs(numerator);
    var positiveDenominator = Math.Abs(denominator);

    var gcd = CalculateGreatestCommonDenominator(positiveNumerator, positiveDenominator);

    int semiFinalNumerator = positiveNumerator / gcd;
    int finalNumerator = isNegative ? -1 * semiFinalNumerator : semiFinalNumerator;
    int finalDenominator = positiveDenominator / gcd;

    return finalDenominator == 1
        ?(finalNumerator, null)
        :(finalNumerator, finalDenominator);
}
  • First of all, do not overwrite your parameters numerator and denominator
    • Use separate variable for each operator with meaningful names
      • You can avoid to capture the What and How
    • It also helps debugging
  • I would encourage you to return a ValueTuple instead of a string
    • It helps the caller to process it later (don't need to parse it)
    • It helps the reusability of your code
  • If one of the preconditions / prerequisites fails then throw exception (e.g.: InvalidOperationExcetpion) than return with a simple string
    • It helps the consumer to decide whether the operation was successful or not
  • You can combine the two return statements into a single one

Main

[Theory]
[InlineData("-24/12", -2, null)]
[InlineData("-3/2", -3, 2)]
[InlineData("3/-1", -3, null)]
[InlineData("-6/-2", 3, null)]
public void HappyPath(string fraction, int expectedDividend, int? expectedDivisor)
{
    //Arrange
    var SUT = new Calculator();

    //Act
    var (dividend, divisor) = SUT.Reduce(fraction);

    //Assert
    Assert.Equal(expectedDividend, dividend);
    Assert.Equal(expectedDivisor, divisor);
}
[Theory]
[InlineData("3/0")]
[InlineData("3/8/1")]
[InlineData("4a/6")]
[InlineData("2/8a")]
[InlineData("////")]
public void UnhappyPath(string fraction)
{
    //Arrange
    var SUT = new Calculator();

    //Act
    Action actualCall = () => SUT.Reduce(fraction);

    //Assert
    var exception = Assert.Throws<InvalidOperationException>(actualCall);
    Assert.Contains(fraction, exception.Message);
}
  • I would encourage you to write unit tests in order to verify your implementation
  • I would also suggest dividing happy and unhappy cases into separate test cases
    • As you can see the unhappy tests can easily decide whether or not the operation has been failed
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for the detailed answer Peter, i really appreciate this. A lot of new concepts for me to dig into, which was pretty much what i was looking for. \$\endgroup\$ Jun 15, 2021 at 12:59
  • \$\begingroup\$ I disagree with the default/fallback distinction, it's arbitrary and not meaningful here. I would fundamentally implement the GCD differently (= using the Euclidean algorithm with recursion) but the use of that nullable mutable variable above makes the code more complex, not less so. \$\endgroup\$ Jun 15, 2021 at 20:18
5
\$\begingroup\$

Not bad.

  1. Add tests and move examples there.

  2. Split long lines:

    if( members.Length != 2
     || !int.TryParse(members[0], out int numerator) 
     || !int.TryParse(members[1], out int denominator) 
     || denominator == 0
    )
    

and lines with several statements:

   if (denominator == 1) 
       return numerator.ToString();
  1. Use Euclidean algorithm for GCD.

  2. The function name should probably be Gcd, not GCD (not sure, this depends on guidelines).

  3. It will look much more C#ish if there will be a class for fractions, to use it like this:

    Fraction fraction = Fraction.FromString(frac).Reduce();
    Console.WriteLine($"{fraction}"); //uses ToString method
    
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your input Pavlo. When you say 'Add tests and move examples there.', you mean create a small Unit Test project? \$\endgroup\$ Jun 15, 2021 at 12:46
  • \$\begingroup\$ Yes, I'm talking about unit test. \$\endgroup\$ Jun 16, 2021 at 13:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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