Calculating a page size to execute a subset of jobs at a time

Question

We have to execute a bunch of jobs , each have an ID and they are passed on to the processor . Since the processor can only work on a limited number of jobs (denoted by deleteBatchSizeMax in the code snippet),we are using a paged approach so that only a subset is picked up by the processor.To implement this we have a functionality which calculates the Page Size. It takes in the total number of jobs, and the max page size. The code handles the situation where the page size may not be an exact multiple , example 22 items have a deleteBatchSizeMax of 10. This translates to 22/10  so it gives us a page size of 3 where the jobs are executed in chunks of 10,10,2.
Hence wrote the below described function!
    private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax) 
=> totalNumberOfJobs % deleteBatchSizeMax != 0
            ? (totalNumberOfJobs / deleteBatchSizeMax) + 1
            : totalNumberOfJobs / deleteBatchSizeMax;

I am hearing complaints that apparently its not readable and I should do the if-else block.  I think that the variable names are well defined , code is well commented (omitted here) and self explanatory.
I wanted to know the feedback of the community.

alexyorke · Accepted Answer

There are a few ways which this method could be made more readable.

The first if-statement is negated, making the control flow slightly more complicated; the statement can be negated (again) to clarify the flow:

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax) 
            => totalNumberOfJobs % deleteBatchSizeMax == 0
            ?  totalNumberOfJobs / deleteBatchSizeMax
            : (totalNumberOfJobs / deleteBatchSizeMax) + 1;

Secondly, the statement is currently checking if totalNumberOfJobs is an even divisible of deleteBatchSizeMax. The computation totalNumberOfJobs / deleteBatchSizeMax is used three times. When (double)totalNumberOfJobs / (double)deleteBatchSizeMax doesn't contain a decimal place, then it implies totalNumberOfJobs % deleteBatchSizeMax == 0, and vice-versa.

To simplify the pattern, consider the following: when A divides B with no remainder, then divide it, otherwise divide it and add one. Therefore, Math.Ceiling can be used:

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax) 
    => (int)Math.Ceiling((double)totalNumberOfJobs / (double)deleteBatchSizeMax);

The Math.Ceiling function "rounds" up to positive infinity; so 4.5 -> 5, 4.00001 -> 5 and so on. This emulates the original loop condition.

Edit: the second double conversion is not required as it is upcasted to a double.

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax) 
    => (int)Math.Ceiling((double)totalNumberOfJobs / deleteBatchSizeMax);

Traycho Ivanov · Answer

I would go with following sample

Mare sure total and count are positive numbers
On next step we expect the pages result to be always positive > 1.

Divide the total number of item by the count. All divisions by total < count leads to 0 that is the reason +1 is aded to the result. By having total-1 we handle the case total = count and having + 1 will give are wrong result. Well this could be handled with few ifs but in the case you prefer a shorter version ...

private int pages(int total, int count){
  if(total <= 0 || count <=0 ){
    return 0;
  }
  return ((total - 1) / count) + 1;
}

Tests:

pages(0,10) -> 0 
pages(1,10) -> 1
pages(10,10)) -> 1
pages(11,10)) -> 2
pages(101,10) -> 11

Roland Illig · Answer

Inspired by alexyorke's answer, what about this:

In a MathUtils class, define:

public static int DivCeiling(int num, int den) => (num + den - 1) / den;

And then, in your application class, define:

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax)
    => MathUtils.DivCeiling(totalNumberOfJobs, deleteBatchSizeMax);

That way you can combine the short variable names of an abstract class for mathematical utilities (since the formula can be used in really many contexts) with the readability and simplicity in your application class.

You can also write extensive unit tests for the MathUtils class, focusing on the formula. And in your application code you only have to decide whether it is correct to use the DivCeiling function or not.

As usual for unit tests, pay attention to edge cases:

num == 0 (always results in 0)
den == 0 (exception, that's ok)
num == int.MaxValue (overflow in my code, but converting the numbers to double feels inappropriate to me)
den == int.MaxValue / 2 + 1, num = int.MaxValue / 2 + 2 (overflow)
num < 0
den < 0

To fix this, you could do the intermediate calculation using long instead of int. You just have to ensure that when converting the result back to int, that you don't cut off any significant digits.

Calculating a page size to execute a subset of jobs at a time

3 Answers

Add your own answers!

Ask a Question