Code Review Asked by user93068 on December 29, 2020

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.

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);
```

Correct answer by alexyorke on December 29, 2020

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
```

Answered by Traycho Ivanov on December 29, 2020

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.

Answered by Roland Illig on December 29, 2020

Get help from others!

Recent Answers

- Joshua Engel on Why fry rice before boiling?
- Peter Machado on Why fry rice before boiling?
- haakon.io on Why fry rice before boiling?
- Lex on Does Google Analytics track 404 page responses as valid page views?
- Jon Church on Why fry rice before boiling?

Recent Questions

- How can I transform graph image into a tikzpicture LaTeX code?
- How Do I Get The Ifruit App Off Of Gta 5 / Grand Theft Auto 5
- Iv’e designed a space elevator using a series of lasers. do you know anybody i could submit the designs too that could manufacture the concept and put it to use
- Need help finding a book. Female OP protagonist, magic
- Why is the WWF pending games (“Your turn”) area replaced w/ a column of “Bonus & Reward”gift boxes?

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP