Divide two numbers, then apply a custom rounding rule

Question

If the fractional part of the number is greater than 0.6, round up; if less, round down. I need feedback about my code.

public static double DivisionMethod(double a, double b)
{
 double div = a / b;
 double temp = Math.Floor(div);
 double fractional = div - temp;
 if (fractional > 0.6)
 {
 return Math.Ceiling(div);
 }
 else
 {
 return Math.Floor(div);
 }
}

• dcastro: Did you mean "if less or equal - round down"?
• dcastro: Also, by round down, do you mean towards zero or towards negative infinity? Would -2.5 round to -2 (towards zero) or -3 (towards negative infinity)?
• Eric Lippert: You might enjoy this question. stackoverflow.com/questions/921180/… The lesson here is that even smart people can get division completely wrong, over and over again. You are very wise to be careful about your code and get it reviewed.
• CodesInChaos: @dcastro The double constant 0.6 isn't 0.6 in the mathematical sense anymore. That your equation evaluates to true is merely luck. There are similar looking equations (such as double d = 1.2d/3d; d == 0.4 that won't do so. I'm not even sure if it your example will work reproducibly across different implementations an hardware.
• Brian: @dcastro: Pulling up MiscUtil.Conversion.DoubleConverter.ToExactString, 0.6d is represented as 0.59999999999999997779553950749686919152736663818359375 (as is 1.2d/2d). Mentally replace all instances of 0.6 with that horrifying number. During output, C# often prints out 0.6 (because it chops of the last few digits for display).

Answer 1

DivisionMethod is a pretty vague/poor name for your method; any method with the word "method" in it should raise a "bad name" flag!

Also, hard-coding the threshold into the function takes away flexibility. Perhaps I'd consider an optional parameter, like this:

public static double RoundedDivision(double a, double b, double threshold = 0.6)

Okay, not sure RoundedDivision is a much better name... naming is hard!. This is where XML comments can help:

/// <summary>
/// Divides <c>a</c> by <c>b</c> and round up if quotient is
/// greater than <c>threshold</c>, round down if it's less. 
/// </summary>
public static double RoundedDivision(double a, double b, double threshold = 0.6)

It's probably best to call a cat, a cat: a is the dividend and b is the divisor, so the method would be clearer with a signature like this:

/// <summary>
/// Divides <c>dividend</c> by <c>divisor</c> and round up if quotient is
/// greater than <c>threshold</c>, round down if it's less. 
/// </summary>
public static double RoundedDivision(double dividend, double divisor, double threshold = 0.6)

As was mentioned in the comments, there's an ambiguity issue with the threshold: what happens if the quotient is exactly equal to the threshold?

Answer 2

First, you're calculating Math.Floor(div) twice. You have already stored it in temp (and why not give it a more clear name, like floor).

Second, you don't need the fractional variable:

if (div - floor > 0.6) etc.

Third, Math.Ceiling in this case is equal to floor + 1;

The rest is a matter of personal style. Some people would remove the braces and some would use a ternary operator instead of if/else, like this:

public static double DivisionMethod(double a, double b)
{
 double div = a / b;
 double floor = Math.Floor(div);
 return div - floor > 0.6 ? floor + 1 : floor;
}

• Snowbody: Math.Ceiling(...) is equal to floor+1... as long as div is not an integer. If it is an integer, they're the same.
• Keen: @Snowbody If div is an integer, div - floor is 0, so floor + 1 is never reached.
• Dennis_E: @Snowbody You're right of course. What I meant was the return value. If floor > 0.6 then you don't have to calculate Math.Ceiling
• Snowbody: @dennis_e Rereading the post it seems that you are saying that only in that specific case, you can guarantee Math.Ceiling() == floor+1 but I didn't pick that up the first time.

Answer 3

The other answers are good. I would add to them:

• Show us your test cases and their results; we can criticize them as well and show you what test cases you should be using.

• Do you find it a little bit strange that DivisionMethod(10, 7) is 1 but DivisionMethod(-10, 7) is -2 ? I would expect that changing the sign of an operand should change the sign of the output, not the sign and magnitude of the output. This is not a question about the action of the code, but rather whether the method is poorly specified to begin with. Any method that deals with division and rounding I expect the specification to say very clearly what to do in the case of negative numbers.

• Note that 0.6 cannot be represented exactly by a double; doubles are fractions where the denominator is a power of two, and three-fifths cannot be made to have a power of two on the bottom. If your intention is to represent exactly fractions which have a power of ten on the bottom, you need to use decimal, not double.

Answer 4

public static double DivisionMethod(double dividend, double divisor)
{
 double div = dividend / divisor;
 double floor = Math.Floor(div);
 return div - floor <= 0.6 ? floor : floor+ 1;
}

There is more probability that div - Math.Floor(div) is smaller then and equal to 0.6. so I think it's a good practise to use

div - floor <= 0.6 ? floor : floor + 1; 

instead of

div - floor > 0.6 ? floor + 1 : floor;

by this reduced the probability of adding 1 to floor.

• Snowbody: especially since the second way of writing it is wrong if div happens to be an integer.

Answer 5

If you want 0.6 -> 0.0 then return Math.Ceiling(x/y - 0.6 )

If you want 0.6 -> 1.0 then return Math.Floor(x/y + 1 - 0.6)

Answer 6

Taking into account previous suggestions, I propose the following, which:

• Provides better names
• Handles divide by zero
• Handles negative outcomes

public static double CustomDivision(double dividend, double divisor, double threshold = 0.6)
{
 // Start with divisor in case it's zero
 double answer = divisor;
 // Avoid divide by zero error
 if (!divisor.Equals(0.0))
 {
 // Perform calculations
 answer = dividend / divisor;
 double floor = Math.Floor(answer);
 double ceiling = Math.Ceiling(answer);
 bool isPositive = answer < 0 ? false : true;
 double remainder = answer - (isPositive ? floor : ceiling);
 // Which way to round depends on sign and remainder
 bool roundUp = isPositive ? 
 (Math.Abs(remainder) > threshold ? true : false) : 
 (Math.Abs(remainder) > threshold ? false : true);
 // Now we know which way to round
 answer = roundUp ? ceiling : floor;
 }
 return answer;
}

• Marc-Andre: Are you asking for a review here ?
• Snowbody: This is a place to review code, not post your own code. What comments do you have about the OP's code?
• ms.spock: OP's code didn't handle negatives and didn't check for divide by zero.
• Jamal: @ms.spock: You should also mention that in the answer, so that it's clear that you're pointing out a flaw in the code. Otherwise, this may be subject to deletion.