4
\$\begingroup\$

Context:

My project has a hand-rolled ORM (... yeah ... I know) ...

built around the MS DataSet objects that represent wrapped collections of rows of DBtables (... yeah ... I know) ...

and which use negative ints to represent identity columns on new Rows which haven't yet been synched with the DB (... yeah ... I know) ...

the upshot of which is we objects that generally expect an identity to start at -1 and be decremented until they reach int.MinValue

Furthermore, we sometimes need to tell these objects that they can only use a certain sub-set of the negative integer range, so that multiple threads can create rows, which can then be combined without risk of ID conflicts. (... yeah ... I know) ...

and occasionally this subdivision has to happen repeatedly :(

All of this is to explain why it is that I need a method that is going to take a range of negative numbers, and sub-divide it. But that these ranges want to be thought of as running from, say, -100 to -999 (i.e. they run the 'wrong way').

Alas, all of this set-up is completely non-negotiable. so the plethora of comments along the lines of "the best way to fix this code is to not need it, or to change what it's trying to do" are (whilst completely correct) not at all useful.

We need to split an arbitrary negative range into n sub-ranges, all of which MUST be strictly dis-joint. It's NOT necessary that we use all the integers (though it's preferable not to waste too many). It's not necessary that we always use the boundary values, though again preferable.

We're guaranteed to be given the range (-1, int.MinValue), so have to support that.

Primary goal of review is primarily to spot bugs and edge cases, and gather views on readibility/understandability of code. Other aesthetic/design comments welcome too. Unit Test suite will be written, given the ease of testing.

So, the code:

public static void IdentifyIndexRangeForDataSetUse(
  int xiExistingRangeStart,
  int xiExistingRangeEnd,
  int xiTotalNumberOfDivisionsBeingMade,
  int xiIndexOfTargetDivision,
  out int xoTargetRangeStart,
  out int xoTargetRangeEnd)
{
  if (xiExistingRangeStart < xiExistingRangeEnd)
  { throw new System.InvalidOperationException("The SMALLER value MUST be provided first. Note that since values are negative, this first value will have the larger ABSOLUTE value!"); }

  if (xiExistingRangeStart > 0 || xiExistingRangeEnd > 0)
  { throw new System.InvalidOperationException("The existing range should lie entirely within the Negative Integers."); }

  // REVERSE THE NUMBERS!!
  // We get given a range like (-1 --> -99), but it's WAY easier to think and reason about the range (1 --> 99)
  // so simply reflect the range into the positive domain and work there.
  int lPosExistingRangeStart, lPosExistingRangeEnd;

  lPosExistingRangeStart = -1 * xiExistingRangeStart;
  if (xiExistingRangeEnd == int.MinValue)
  {
    lPosExistingRangeEnd = int.MaxValue;
  }
  else
  {
    lPosExistingRangeEnd = -1*xiExistingRangeEnd;
  }


  var lTotalLengthOfExistingRange = lPosExistingRangeEnd - lPosExistingRangeStart + 1; //check overflows
  var lTotalLengthOfNewRanges = lTotalLengthOfExistingRange / xiTotalNumberOfDivisionsBeingMade;
  int lPosTargetRangeStart = lPosExistingRangeStart + (xiIndexOfTargetDivision * lTotalLengthOfNewRanges);
  int lPosTargetRangeEnd = lPosTargetRangeStart + (lTotalLengthOfNewRanges - 1); //the '-1' prevents overlaps.


  // REVERSE THE NUMBERS!!
  // To take it back into the negative domain.
  xoTargetRangeStart = -1 * lPosTargetRangeStart;
  xoTargetRangeEnd = -1 * lPosTargetRangeEnd;

}
\$\endgroup\$
  • \$\begingroup\$ What kind of Hungarian notation is that? I've never seen xi and xo before... Something to do with in vs out? \$\endgroup\$ – RobH Sep 28 '16 at 11:09
  • \$\begingroup\$ yeah ... l = Local, m=Member, xi = parameter in, xb = parameter by ref, xo = parameter out, f = lamba function variable. Bearing in mind that this codebase regularly has 500+ line functions or 5k+ line classes, it can be really helpful. (In case it's not obvious, this is a 15 year old monstrosity :) ) \$\endgroup\$ – Brondahl Sep 28 '16 at 11:20
  • 1
    \$\begingroup\$ 500+ line functions or 5k+ line classes wooow :-O \$\endgroup\$ – t3chb0t Sep 28 '16 at 16:14
2
\$\begingroup\$

Redundant check

If xiExistingRangeStart is greater than xiExistingRangeEnd, then you only have to verify that xiExistingRangeStart is not greater than zero to know that xiExistingRangeEnd can't be.

Parameters + Error messages

I would prefer your exception messages to be more explicit. Rather than:

"The SMALLER value MUST be provided first. Note that since values are negative, this first value will have the larger ABSOLUTE value!"); }

I think it would be better to refer to 'Start' rather than 'first' value. This may seem pedantic but consistency between the error name and the parameter name make it easier to resolve issues.

Similarly, when I think of negative numbers, I tend to think of numbers < 0, not <= zero which seems to be what your code suggests. So again, I'd tend to be more explicit.

\$\endgroup\$
  • \$\begingroup\$ All accurate points. I'm inclined to keep the redundant check, but the error message is changed, and the >0 is now > -1 (also prevents an overflow on lTotalLengthOfExistingRange) \$\endgroup\$ – Brondahl Sep 28 '16 at 10:37
1
\$\begingroup\$

If I get it right your main problem is to create id ranges.

I won't review your code because @forsvarir has already given some good advice but I will suggest a different approach.

Have you heard about the Custom Partitioners for PLINQ and TPL?

For example:

var partitioner = Partitioner.Create(0, 100, 20);

partitioner.AsParallel().ForAll(idRange => 
{   
    Console.WriteLine(idRange );
});

You would need to negate the numbers but this should make the whole paritioning much easier. Inside the parallel loop you can work with each range separately.

This is what you'll get (you should treat the Item2 as exclusive so don't worry about overlapping ranges)

Partitions

\$\endgroup\$
1
\$\begingroup\$

There is one case that your code doesn't handle correctly:

int start, end;
IdentifyIndexRangeForDataSetUse(int.MinValue, int.MinValue, 10, 1, out start, out end);

// start == -2147483648
// end == -2147483647

IdentifyIndexRangeForDataSetUse(-1, -1, 10, 1, out start, out end);
// start == -1
// end == 0

I think you should check that the range isn't zero length and throw an exception if it is.

Use the ternary operator where it makes sense:

lPosExistingRangeEnd = xiExistingRangeEnd == int.MinValue ? int.MaxValue : -1*xiExistingRangeEnd;

I'd also prefer Math.Abs or the unary negation operator over the explicit multiplication. I'd also factor it out as a separate method.

private int TransformToPositiveDomain(int number)
{
    return number == int.MinValue ? int.MaxValue : Math.Abs(number);
}

And likewise for transforming back:

private int TransformToNegativeDomain(int number)
{
    return number > 0 ? number * -1 : number;
}

Passing around so many separate ints is also an issue for me. Either use a Tuple<int, int> or create a Range class:

public class IntegerRange
{
    public Start { get; set; }
    public End { get; set; }
}

You could even implement IEnumerable and allow for foreaching through the range.


That makes your signature much clearer:

public IntegerRange GetIndexRange(IntegerRange existingRange, int numberOfDivisions, int rangeIndex)

Oh, you also need to validate your other parameters:

int start, end;
IdentifyIndexRangeForDataSetUse(-1, -20, -10, 1, out start, out end);

// start == 1
// end == 4

You should validate the total divisions and division index are greater than 0.


As an aside, age of codebase is no excuse for it being poor. The codebase I work on is only a few years younger than yours and we don't have monstrous classes or functions that are hundreds of lines long. It's never too late to start refactoring ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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