tags:

views:

12582

answers:

5

I want to ensure that a division of integers is always rounded up if necessary, is there a better way than this, there is a lot of casting going on :-)

(int)Math.Ceiling((double)myInt1 / myInt2)
A: 

You could use something like the following.

a / b + ((Math.Sign(a) * Math.Sign(b) > 0) && (a % b != 0)) ? 1 : 0)
Daniel Brückner
That's a nice one, although using (int)(float + 0.5f) could be a bit easier to understand. Also, it uses a division, a modulo, an add and an if (or somewhat similar), so it's damn slow :)
schnaader
But it uses only integer operations and they are damn fast compared to floating point operations.
Daniel Brückner
But I am not yet satisfied - I believe there is a very smart integer arithmetic hack ... going to think about it a bit ...
Daniel Brückner
Good point - also I didn't realize that the division will be done in any case, and an integer division is much faster than a float division.
schnaader
This code is obviously wrong in two ways. First off, there is a minor error in syntax; you need more parentheses. But more importantly, it does not compute the desired result. For example, try testing with a=-1000000 and b = 3999. The regular integer division result is -250. The double division is -250.0625... The desired behaviour is to round up. Clearly the correct rounding up from -250.0625 is to round up to -250, but your code rounds up to -249.
Eric Lippert
You are right. Going to fix the parenthesis and the logic. I messed the logic - should be smaller than zero and or instead of and.
Daniel Brückner
Inverting the whole expression makes it more understandable, I think. If a * b > 0 and a % b != 0, add one.
Daniel Brückner
The code is still wrong. Try it with a = +1000000 and b = +3999. The integer division is 250, the double division is 250.06, so it should be rounded up to 251. Your code gives the result of 250. Why are you multiplying? Multiplication gives you nothing useful. If what you want to do is compare the sign of "a" to the sign of "b" then surely the sensible way to do that is ((a>0)==(b>0)), no?
Eric Lippert
The code is theoretical correct, but I missed that the multiplication overflows. I used the multiplication to avoid another division and because the adjustment is required if a < 0 and b < 0 or a > 0 or b > 0. For this a * b > 0 was the shortest correct solution. ((a > 0) == (b > 0)) fails (at least) for a == 0 and b == 0.
Daniel Brückner
(1) using "a*b>0" is not the _shortest correct solution_ because it is _not a correct solution_. and (2), if a==0 and b==0 then you just divided zero by zero, so you've already thrown an exception.
Eric Lippert
I'm sorry to have to keep saying this but your code is STILL WRONG Daniel. 1/2 should round UP to 1, but your code rounds it DOWN to 0. Every time I find a bug you "fix" it by introducing another bug. My advice: stop doing that. When someone finds a bug in your code, don't just slap together a fix without thinking through clearly what caused the bug in the first place. Use good engineering practices; find the flaw in the algorithm and fix it. The flaw in all three incorrect versions of your algorithm is that you are not correctly determining when the rounding was "down".
Eric Lippert
Unbelievable how many bugs can be in this small piece of code. I did never have much time to think about it - the result manifests in the comments. (1) a * b > 0 would be correct if it did not overflow. There are 9 combinations for the sign of a and b - [-1, 0, +1] x [-1, 0, +1]. We can ignore the case b == 0 leaving the 6 cases [-1, 0, +1] x [-1, +1]. a / b rounds towards zero, that is rounding up for negative results and rounding down for positve resuls. Hence the adjustment must be performed if a and b have the same sign and are not both zero.
Daniel Brückner
a * b > 0 does exactly this because a * b is positive if and only if a and b are both positive or both negative. (2) The bug introduced by the devision is the same that would occur in the code you posted in a removed comment (result = a / b; if ((result > 0) ) because the sign is not correctly detected if the divisions rounds down to zero. (c) You are right with my comment on ((a > 0) == (b > 0)). But I wrote at least and indeed it fails for a = 0 and b < 0.
Daniel Brückner
(0 > 0) == (-1 > 0) is obviously true, but 0 / -1 should not be adjusted. Actually the second test - a % b != 0 - will evaluate to false and in turn no adjustment will be done, but it does not express the idea very well.
Daniel Brückner
Too clever by half. Is it understandable?
Jeremy McGee
This answer is probably the worst thing I wrote on SO ... and it is now linked by Eric's blog... Well, my intention was not to give a readable solution; I was really locking for a short and fast hack. And to defend my solution again, I got the idea right the first time, but did not think about overflows. It was obviously my mistake to post the code without writing and testing it in VisualStudio. The "fixes" are even worse - I did not realize that it was an overflow problem and thought I made an logical mistake. In consequence the first "fixes" did not change anything; I just inverted the
Daniel Brückner
logic and pushed the bug around. Here I made the next mistakes; as Eric already mentioned I did not really analyze the bug and just did the first thing that seemed right. And I still did not use VisualStudio. Okay, I was in a hurry and did not spend more then five minutes on the "fix", but this should not be an excuse. After I Eric repeatedly pointed out the bug, I fired up VisualStudio and found the real problem. The fix using Sign() makes the thing even more unreadable and turns it into code you don't really want to maintain. I learned my lesson and will no longer underestimate how tricky
Daniel Brückner
seemingly simple things may become.
Daniel Brückner
+17  A: 

You could write a helper.

static int DivideRoundUp(int p1, int p2) {
  return (int)Math.Ceiling((double)p1 / p2);
}
JaredPar
Still the same amount of casting going on though
ChrisF
Presumably the OP was smart enough to put this in a function already...
Outlaw Programmer
@Outlaw, presume all you want. But for me if they don't put it into the question, I generally assume they didn't consider it.
JaredPar
+13  A: 

Perfect chance to use an extension method:

public static class Int32Methods
{
    public static int DivideByAndRoundUp(this int number, int divideBy)
    {                        
        return (int)Math.Ceiling((float)number / (float)divideBy, 0);
    }
}

This makes your code uber readable too:

int result = myInt.DivideByAndRoundUp(4);
joshcomley
+1 for extension method
P.K
Erm, what? Your code would be called myInt.DivideByAndRoundUp() and would always return 1 except for an input of 0 which would cause an Exception...
configurator
@configurator - Good point, I've fixed the code!
joshcomley
Epic failure. (-2).DivideByAndRoundUp(2) returns 0.
Timwi
I'm really late to the party, but does this code compile? My Math class does not contain a Ceiling method that takes two arguments.
Martinho Fernandes
+16  A: 

The final int-based answer

For signed integers:

int div = a / b;
if (((a ^ b) >= 0) && (a % b != 0))
    div++;

For unsigned integers:

int div = a / b;
if (a % b != 0)
    div++;

The reasoning for this answer

Integer division '/' is defined to round towards zero (7.7.2 of the spec), but we want to round up. This means that negative answers are already rounded correctly, but positive answers need to be adjusted.

Non-zero positive answers are easy to detect, but answer zero is a little trickier, since that can be either the rounding up of a negative value or the rounding down of a positive one.

The safest bet is to detect when the answer should be positive by checking that the signs of both integers are identical. Integer xor operator '^' on the two values will result in a 0 sign-bit when this is the case, meaning a non-negative result, so the check (a ^ b) >= 0 determines that the result should have been positive before rounding. Also note that for unsigned integers, every answer is obviously positive, so this check can be omitted.

The only check remaining is then whether any rounding has occurred, for which a % b != 0 will do the job.

Lessons learned

Arithmetic (integer or otherwise) isn't nearly as simple as it seems. Thinking carefully required at all times.

Also, although my final answer is perhaps not as 'simple' or 'obvious' or perhaps even 'fast' as the floating point answers, it has one very strong redeeming quality for me; I have now reasoned through the answer, so I am actually certain it is correct (until someone smarter tells me otherwise -furtive glance in Eric's direction-).

To get the same feeling of certainty about the floating point answer, I'd have to do more (and possibly more complicated) thinking about whether there is any conditions under which the floating-point precision might get in the way, and whether Math.Ceiling perhaps does something undesirable on 'just the right' inputs.

The path travelled

Replace (note I replaced the second myInt1 with myInt2, assuming that was what you meant):

(int)Math.Ceiling((double)myInt1 / myInt2)

with:

(myInt1 - 1 + myInt2) / myInt2

The only caveat being that if myInt1 - 1 + myInt2 overflows the integer type you are using, you might not get what you expect.

Reason this is wrong: -1000000 and 3999 should give -250, this gives -249

EDIT:
Considering this has the same error as the other integer solution for negative myInt1 values, it might be easier to do something like:

int rem;
int div = Math.DivRem(myInt1, myInt2, out rem);
if (rem > 0)
  div++;

That should give the correct result in div using only integer operations.

Reason this is wrong: -1 and -5 should give 1, this gives 0

EDIT (once more, with feeling):
The division operator rounds towards zero; for negative results this is exactly right, so only non-negative results need adjustment. Also considering that DivRem just does a / and a % anyway, let's skip the call (and start with the easy comparison to avoid modulo calculation when it is not needed):

int div = myInt1 / myInt2;
if ((div >= 0) && (myInt1 % myInt2 != 0))
    div++;

Reason this is wrong: -1 and 5 should give 0, this gives 1

(In my own defence of the last attempt I should never have attempted a reasoned answer while my mind was telling me I was 2 hours late for sleep)

jerryjvl
No, this is ALSO wrong. Suppose myInt1 is -1, myInt2 is -5. The integer division is zero. The double division is 0.2. The remainder is -1. The correct answer is to round zero up to one, but this does not, because the remainder is negative.
Eric Lippert
And, of course you are right... :) ... should teach me for writing it up quickly.
jerryjvl
And I just spotted after my last edit that Daniel came to almost the same result... altho I think the `>=0` instead of '>0' is important to catch the boundary case Eric just highlighted in my incorrect solution.
jerryjvl
No, this code is STILL WRONG. What if myInt1 is -1 and myInt2 is 2? The integer division is zero, the remainder is nonzero, and you are now rounding -0.5 off to 1. People, please, randomly changing the code every time someone finds a bug is not a good engineering practice!
Eric Lippert
I wonder why this still keeps getting occasional down-votes ... the actual answer at the top of the post has been correct since the last edit... it probably is due to the last Eric Lippert comment here which refers to the last version in the history at the bottom.
jerryjvl
+1 (once I get my votes back tomorrow). I like how you keep all the mistakes in the answer so we can see how easy it is to mess things up when you don't stop to think about it.
Martinho Fernandes
@Martinho: And how deceptively simple something subtle can look...
jerryjvl
Blimey... this post made me think way harder about how/when this seemly easy op was going to blow up. Thanks all!!
argatxa
+314  A: 

Getting integer arithmetic correct is hard. As has been demonstrated amply thus far, the moment you try to do a "clever" trick, odds are good that you've made a mistake. And when a flaw is found, changing the code to fix the flaw without considering whether the fix breaks something else is not a good problem-solving technique. So far we've had I think five different incorrect integer arithmetic solutions to this completely not-particularly-difficult problem posted.

The right way to approach integer arithmetic problems -- that is, the way that increases the likelihood of getting the answer right the first time - is to approach the problem carefully, solve it one step at a time, and use good engineering principles in doing so.

Start by reading the specification for what you're trying to replace. The specification for integer division clearly states:

  1. The division rounds the result towards zero

  2. The result is zero or positive when the two operands have the same sign and zero or negative when the two operands have opposite signs

  3. If the left operand is the smallest representable int and the right operand is –1, an overflow occurs. [...] it is implementation-defined as to whether [an ArithmeticException] is thrown or the overflow goes unreported with the resulting value being that of the left operand.

  4. If the value of the right operand is zero, a System.DivideByZeroException is thrown.

What we want is an integer division function which computes the quotient but rounds the result UPWARDS, not TOWARDS ZERO.

So write a specification for that function. Our function int DivRoundUp(int dividend, int divisor) must have behaviour defined for every possible input. That undefined behaviour is deeply worrying, so let's eliminate it. We'll say that our operation has this specification:

  1. operation throws if divisor is zero

  2. operation throws if dividend is int.minval and divisor is -1

  3. if there is no remainder -- division is 'even' -- then the return value is the integral quotient

  4. Otherwise it returns the smallest integer that is greater than the quotient, that is, rounds up.

Now we have a specification, so we know we can come up with a testable design. Suppose we add an additional design criterion that the problem be solved solely with integer arithmetic, rather than computing the quotient as a double, since the "double" solution has been explicitly rejected in the problem statement.

So what must we compute? Clearly, to meet our spec while remaining solely in integer arithmetic, we need to know three facts. First, what was the integer quotient? Second, was the division free of remainder? And third, if not, was the integer quotient computed by rounding up or down?

Now that we have a specification and a design, we can start writing code.

public static int DivRoundUp(int dividend, int divisor)
{
  if (divisor == 0 ) throw ...
  if (divisor == -1 && dividend == Int32.MinValue) throw ...
  int roundedTowardsZeroQuotient = dividend / divisor;
  bool dividedEvenly = (dividend % divisor) == 0;
  if (dividedEvenly) 
    return roundedTowardsZeroQuotient;

  // At this point we know that divisor was not zero 
  // (because we would have thrown) and we know that 
  // dividend was not zero (because there would have been no remainder)
  // Therefore both are non-zero.  Either they are of the same sign, 
  // or opposite signs. If they're of opposite sign then we rounded 
  // UP towards zero so we're done. If they're of the same sign then 
  // we rounded DOWN towards zero, so we need to add one.

  bool wasRoundedDown = ((divisor > 0) == (dividend > 0));
  if (wasRoundedDown) 
    return roundedTowardsZeroQuotient + 1;
  else
    return roundedTowardsZeroQuotient;
}

Is this clever? No. Beautiful? No. Short? No. Correct according to the specification? I believe so, but I have not fully tested it. It looks pretty good though.

We're professionals here; use good engineering practices. Research your tools, specify the desired behaviour, consider error cases first, and write the code to emphasize its obvious correctness. And when you find a bug, consider whether your algorithm is deeply flawed to begin with before you just randomly start swapping the directions of comparisons around and break stuff that already works.

Eric Lippert
Excellent exemplary answer
Gavin Miller
Ack... I should have looked up in the thread before writing out my post just then :)
jerryjvl
Would you really consider the unspecified behaviour for int.MinValue and -1 undesirable since the specification says it only occurs in unchecked contexts? Surely if the program is executing a division within an unchecked context we should assume that they are specifically trying to avoid the exception?
jerryjvl
What I care about is not the behaviour; either behaviour seems justifiable. What I care about is that it's not _specified_, which means it cannot easily be tested. In this case, we're defining our own operator, so we can specify whatever behaviour we like. i don't care whether that behaviour is "throw" or "don't throw", but I do care that it be stated.
Eric Lippert
That's a good point. Out of curiosity, do you have any idea why the second case may have been left unspecified in the specification of the language of all things?
jerryjvl
I suspect the first two sentences of this answer could be applied to a vast array of topics by replacing "integer arithmetic" with the name of the topic. As a matter of pedantry, I'd also consider changing the specification from "greater than the quotient" to "greater than or equal to the quotient". Otherwise DivRoundUp(12, 2) should return 7, as the smallest integer greater than 6.
Jon Skeet
@Jon: DivRoundUp(12, 2) falls into case (3) of the spec, not case (4).
Eric Lippert
@jerryjvl: I'm not certain, but my _guess_ would be that some hardware makes it cheap to detect this case and some does not.
Eric Lippert
Darn it, pedantry fail :(
Jon Skeet
Great one. I learned a lot about algorithm design by this answer.
Johannes Schaub - litb
Man - Could you write a book on that, please?
xtofl
I've learned from rounding issues in VBA and MySQL pre 5.0 (which I both believe had rounding issues based on problems with the way C implemented rounding) never to rounding as implemented in a language (though perhaps it has gotten better). I almost always end up writing my own methods or operators so I know *exactly* what to expect in every situation. Well done.
Ben McCormack
I'm late to the party - nice analysis and exposition.
Michael Burr
I think this is one of the best answers on the entire of SO.
Iain Galloway
This shakes me at my core - Jon Skeet..wrong? In any case, good answer.
Graphain
Awesomeness written all over the post!!
Ravi Gummadi