Bug: Identification number mismatch

Recently, a developer had fixed a bug with title similar to “Tax Number is not populating in Save Registration dialog”

The original code looked like this:

public CarIdentification CarIdentification1
{
   get { return _carIdentification1; }
}
 
public CarIdentification CarIdentification2
{
   get { return _carIdentification2; }
}

if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
   _carIdentification1.Value = transaction.TaxNumber;
}
 
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
   _carIdentification1.Value = transaction.RegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
   _carIdentification1.Value = transaction.NewRegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
   _carIdentification2.Value = transaction.NewTaxNumber;
}

So a quick glance at the code and you see that 3 of the 4 statements are using “_carIdentification1” then the last one sets _carIdentification2. So without putting much thought into this, I would expect one of the first 3 should actually be using _carIdentification2. But what does 1 or 2 actually mean? Are they representing TaxNumber and RegistrationNumber, or is it representing Old and New?

If it is the former, I think _carIdentification2 would represent TaxNumber so the first one was wrong.

If the latter, then I think _carIdentification2 would represent New so the third one is wrong.

I do know that the concept of TaxNumber was added into the system later on, so most likely it would be: “_carIdentification2 would represent TaxNumber”

However, the developer, Govind had changed it to the following:

public CarIdentification CarIdentification1
{
get { return _taxNumber; }
}
 
public CarIdentification CarIdentification2
{
get { return _registrationNumber; }
}

if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
   _taxNumber.Value = transaction.TaxNumber;
}
 
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
   _registrationNumber.Value = transaction.RegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
   _registrationNumber.Value = transaction.NewRegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
   _taxNumber.Value = transaction.NewTaxNumber;
}

So they have named the fields _taxNumber and _registrationNumber which is much clearer. Notice though that they have deemed carIdentification1 to be _taxNumber which means they are saying the second, third, and fourth statements were wrong!

An additional thought: if a “transaction” comes into the system, what fields do you expect populated? if RegistrationNumber and NewRegistrationNumber are populated, then _registrationNumber will initially be set to RegistrationNumber, then instantly changed to NewRegistrationNumber! So I’d say this logic needs to be if/else so it only sets _registrationNumber once and _taxNumber once.

I point this out to the developer, but I find a large number of Indian developers just seem to respond with a sequence of meaningless words and you have no idea what they are thinking.

Me:

if NewTaxNumber and TaxNumber are specified, won't it just use NewTaxNumber?


Govind:

taxNumber identifier has to updated with new taxNumber value for transactions


Me:

if it is intentionally written this way, it should be if/else

and if it wasn't intended that way, then there is a bug here, similar to the one you are fixing

They ignored me and checked it in. I don’t know if just the original code was mad, or if this new code is mad too. Let’s hope it doesn’t cause more problems when it goes live.