Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
      }
}
share|improve this question
5  
Did you mean "if less or equal - round down"? –  dcastro Jun 26 at 15:04
3  
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)? –  dcastro Jun 26 at 15:11
2  
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. –  Eric Lippert Jun 26 at 18:40
1  
@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. –  CodesInChaos Jun 27 at 9:44
1  
@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). –  Brian Jun 27 at 13:27

6 Answers 6

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?

share|improve this answer

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;
}
share|improve this answer
3  
Math.Ceiling(...) is equal to floor+1... as long as div is not an integer. If it is an integer, they're the same. –  Snowbody Jun 26 at 16:22
1  
@Snowbody If div is an integer, div - floor is 0, so floor + 1 is never reached. –  Cory Jun 26 at 16:53
    
@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 –  Dennis_E Jun 26 at 16:56
    
@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. –  Snowbody Jun 26 at 16:58

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.

share|improve this answer
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.

share|improve this answer
2  
especially since the second way of writing it is wrong if div happens to be an integer. –  Snowbody Jun 26 at 16:24

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)

share|improve this answer

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;
}
share|improve this answer
1  
Are you asking for a review here ? –  Marc-Andre Jun 27 at 18:08
1  
This is a place to review code, not post your own code. What comments do you have about the OP's code? –  Snowbody Jun 27 at 18:10
    
OP's code didn't handle negatives and didn't check for divide by zero. –  ms.spock Jun 27 at 21:49
    
@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. –  Jamal Jun 28 at 2:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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