Number padding in Indian.NET

bjolling

I'm currently doing a code review for a fixed price project that has been developed off-shore by our Indian friends. Their code is riddled with stupid algorithms and misunderstandings of basic patterns such as MVC. Worst thing about all this is that I cannot reject their code unless it really REALLY breaks a requirement. We are currently in crunch mode and should have delivered long time ago.

This is how they pad their numbers with leading zeroes, which obviously is done in one of the services, not in the view where I would have done this.

        #region [Instance Variable]
    private const string LeadingHomePrefix = "2";

    #endregion [Instance Variable]


    #region [Private Member]
    /// <SUMMARY>
    /// Adds leading zeros
    /// </SUMMARY>
    /// <PARAM name="pricingNumber"></PARAM>
    /// <RETURNS></RETURNS>
    private int GetPaddedNumber(int pricingNumber)
    {
        Int16 maxLength = 9;            
        string pNumber = Convert.ToString(pricingNumber);
        StringBuilder strBuilder = new StringBuilder();
        for (int i = 1; i &lt; (maxLength - pNumber.Length); i++)
        {
            strBuilder.Append("0");
        }

        return Convert.ToInt32(LeadingHomePrefix + strBuilder.ToString() + pNumber);
    }

    /// <SUMMARY>
    /// Removes Padding
    /// </SUMMARY>
    /// <PARAM name="pricingNumber"></PARAM>
    /// <RETURNS></RETURNS>
    private int RemovePaddingFromPricingNumber(int pricingNumber)
    {
        string pNumber = Convert.ToString(pricingNumber);

        if (pNumber.Length == 9)
        {
            pNumber = pNumber.Substring(1);
        }

        return Convert.ToInt32(pNumber);
    }

PJH

@bjolling said:

Worst thing about all this is that I cannot reject their code unless it really REALLY breaks a requirement.

"Easily maintainable," clearly not being one of them?

@bjolling said:

We are currently in crunch mode and should have delivered long time ago.

Nor delivered in good time it would seem.

Just askin'.... I know I'm preaching to the converted.

Dare I ask if they're going to be used again?

bjolling

@PJH said:

@bjolling said:
Worst thing about all this is that I cannot reject their code unless it really REALLY breaks a requirement.
"Easily maintainable," clearly not being one of them?

Maintainability suffers because not hitting the revised milestones is considered worse by the business. Maintaince will be offshored as well but to a different center.

@PJH said:

Dare I ask if they're going to be used again?

They are. :-(

Management claims they'll do better next time because they know the framework now!

Rootbeer

There is nothing unique to India in this horrible, horrible code.

I was hoping to see something like an "add digit separators" function that was hardcoded to use the Indian system (1,00,000,00,00,000, "one Lakh Crore") instead of the Western convention of thousands (1,000,000,000,000, "one trillion") but no.

bjolling

@Rootbeer said:

There is nothing unique to India in this horrible, horrible code.

I was hoping to see something like an "add digit separators" function that was hardcoded to use the Indian system (1,00,000,00,00,000, "one Lakh Crore") instead of the Western convention of thousands (1,000,000,000,000, "one trillion") but no.

Except that this is representative of the quality of the rest of their code (all written in India)

I can try to dig up one of those if you want. We have some issues with decimal comma versus decimal point on the UI.

topspin

@bjolling said:

if (pNumber.Length == 9)

What?

emurphy

@bjolling said:

Management claims they'll do better next time because they know the framework now!

At my first programming job, there was a story about an applicant who failed their programming test, and later re-applied with a new claim of "exposure to <relevant language>" - which exposure turned out to consist entirely of failing the test the first time. Fortunately, these guys were sane and didn't hire him the second time either.

TGV

@topspin said:

@bjolling said:
if (pNumber.Length == 9)
What?

And then replacing it by Substring(1). Magical. No nine number digits for these people...

toth

@bjolling said:

return Convert.ToInt32(LeadingHomePrefix + strBuilder.ToString() + pNumber);

I don't get it--what is the LeadingHomePrefix supposed to do? And why does it return an int instead of a string? And how is this different from 2000000000 + pricingNumber (I may be off by one or two zeroes)?

Ryde

@toth said:

I don't get it--what is the LeadingHomePrefix supposed to do? And why does it return an int instead of a string? And how is this different from 2000000000 + pricingNumber (I may be off by one or two zeroes)?

Its added so that when they convert it back into an integer, the leading zeros are "preserved". Otherwise, something like 0000000002 would be turned to 2, making the whole method call useless. Likely they're chopping off the 10th digit somewhere in the consuming code. Why they didn't just return a string, or separate representation from presentation altogether, is beyond me.

Paddles

@TGV said:

@topspin said:
@bjolling said:
if (pNumber.Length == 9)
What?
And then replacing it by Substring(1). Magical. No nine number digits for these people...

Which leads to a test case to allow rejection of the code. Test various numeric fields with nine-digit numbers and watch them collapse.

Giving them more business is the bigger WTF; the reasoning that they now understand, well, anything, is demonstrably wrong. If they had learned how to do things well then they would have applied them in the current release. If someone does a bad job and you reward them by paying them to do more work, it would be logical for them to conclude that what they had been doing was good.

Made me think... how does management in your business deal with internal staff that underperform? If they give formative feedback etc. to help the staff member improve, good. If they sack the underperforming staff member at first opportunity, it's a big case of cognitive dissonance to sack underperforming staff you've invested in but reward underperforming suppliers that you can easily replace. If they promote the underperforming staff member into management, then I take it all back, their actions are entirely consistent.

DOA

@emurphy said:

At my first programming job, there was a story about an applicant who failed their programming test, and later re-applied with a new claim of "exposure to <relevant language>" - which exposure turned out to consist entirely of failing the test the first time. Fortunately, these guys were sane and didn't hire him the second time either.

Did he apply a third time? It sounds like he's getting more and more exposure as time passes

The_Assimilator

This code was written in under a minute and probably works better than their implementation:

private int GetPaddedNumber(int pricingNumber)

{

return Convert.ToInt32('2' + pricingNumber.ToString().PadLeft(8, '0'));

}

XIU

@The_Assimilator said:

This code was written in under a minute and probably works better than their implementation:


private int GetPaddedNumber(int pricingNumber)

{

return Convert.ToInt32('2' + pricingNumber.ToString().PadLeft(8, '0'));

}

You mean,

200000000 + pricingNumber

bjolling

@XIU said:

@The_Assimilator said:
This code was written in under a minute and probably works better than their implementation:


private int GetPaddedNumber(int pricingNumber)
{
 return Convert.ToInt32('2' + pricingNumber.ToString().PadLeft(8, '0'));
}

You mean, 
200000000 + pricingNumber

Yesterday evening, I decided to file a bug report anyway (before reading your post) and I indeed asked to replace it with

private const int Prefix = 200000000;

private static int GetPaddedNumber(int pricingNumber)
{
  if (pricingNumber > 99999999)
    throw new ArgumentOutOfRangeException("pricingNumber", "The Pricing Number cannot be bigger than 99999999");


  return pricingNumber + Prefix;
}

private static int RemovePaddingFromPricingNumber(int pricingNumber)
{
  if (pricingNumber < Prefix || pricingNumber > 99999999 + Prefix)
    throw new ArgumentOutOfRangeException("pricingNumber", "The padded Pricing Number must be between 200000000 and 299999999");

  return pricingNumber - Prefix;
}

derula

@XIU said:

@The_Assimilator said:
This code was written in under a minute and probably works better than their implementation:


private int GetPaddedNumber(int pricingNumber)

{

return Convert.ToInt32('2' + pricingNumber.ToString().PadLeft(8, '0'));

}

You mean, 
200000000 + pricingNumber

GetPaddedNumber(100000000) = 2100000000

200000000 + 100000000 = 300000000

2100000000 != 300000000 q.e.d.

PJH

@bjolling said:

private const int Prefix = 200000000;

private static int GetPaddedNumber(int pricingNumber)
{
  if (pricingNumber > 99999999)
    throw new ArgumentOutOfRangeException("pricingNumber", "The Pricing Number cannot be bigger than 99999999");


  return pricingNumber + Prefix;
}

private static int RemovePaddingFromPricingNumber(int pricingNumber)
{
  if (pricingNumber < Prefix || pricingNumber > 99999999 + Prefix)
    throw new ArgumentOutOfRangeException("pricingNumber", "The padded Pricing Number must be between 200000000 and 299999999");

  return pricingNumber - Prefix;
}

I know I'm being picky, but shouldn't

the 99...999 numbers be based off Prefix for when(if?) it changes
likewise, the hard coded strings

bjolling

@Ryde said:

@toth said:
I don't get it--what is the LeadingHomePrefix supposed to do? And why does it return an int instead of a string? And how is this different from 2000000000 + pricingNumber (I may be off by one or two zeroes)?

Its added so that when they convert it back into an integer, the leading zeros are "preserved". Otherwise, something like 0000000002 would be turned to 2, making the whole method call useless. Likely they're chopping off the 10th digit somewhere in the consuming code. Why they didn't just return a string, or separate representation from presentation altogether, is beyond me.

We are in the insurance business and the prefix indicates what domain the quote was made for. For vehicle insurance, the prefix is "1", for property it is "2" and so on...

All quotes are saved in the same DB2 table on mainframe. The key is the pricingNumber padded with the prefix. This prefix is then used to know the domain the quote was made for.

bjolling

@PJH said:

I know I'm being picky, but shouldn't

the 99...999 numbers be based off Prefix for when(if?) it changes

likewise, the hard coded strings

Under normal circumstances I would say you are right. But this code is copy/pasted in different applications (one for each insurance domain: property, vehicle - another wtf) and with the deadlines quickly approaching, I'm not going to ask them to refactor and move these functions to a common assembly.
I'm allowing a quick copy/paste for the sake of meeting the deadline

@PJH said:

@bjolling said:
code
I know I'm being picky, but shouldn't

the 99...999 numbers be based off Prefix for when(if?) it changes

likewise, the hard coded strings

It's not being picky, but smart.

It's both easier to read (at least for me it is) and mantain if you use (PREFIX * (10 ** NUMBER_OF_PLACES)) and ((10 ** NUMBER_OF_PLACES) - 1).

(extra parenthesis for extra clarity

EDIT:@bjolling said:

Under normal circumstances I would say you are right. But this code is copy/pasted in different applications (one for each insurance domain: property, vehicle - another wtf) and with the deadlines quickly approaching, I'm not going to ask them to refactor and move these functions to a common assembly.
I'm allowing a quick copy/paste for the sake of meeting the deadline

Fair enough.

XIU

@derula said:

GetPaddedNumber(100000000) = 2100000000

200000000 + 100000000 = 300000000

2100000000 != 300000000 q.e.d.

True, but the RemovePadding method checked for length == 9 and the code that is now used shows that I can never be bigger then 100000000-1.

toth

@Ryde said:

Its added so that when they convert it back into an integer, the leading zeros are "preserved".

That's exactly what I suspected. Ew.