# Function that receives a string fraction and has to return said fraction reduced

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; } }  • What does your code output for Reduce("4/2"), Reduce("4/-6"), Reduce("-6/-2"), or any ill formated fractions involving 0 or infinity? Jun 15, 2021 at 22:54 ## 2 Answers ### 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
• 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. Jun 15, 2021 at 12:59
• 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. Jun 15, 2021 at 20:18

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

• Thanks for your input Pavlo. When you say 'Add tests and move examples there.', you mean create a small Unit Test project? Jun 15, 2021 at 12:46
• Yes, I'm talking about unit test. Jun 16, 2021 at 13:36