The takeaway from this is to Fail Fast.
We don't have the code, nor do we have many examples of prefixes that are or are not test branch prefixes according to the code. All we have is this:
- 089 - 100 => test branch
- 10B, 10C => test branch
- < 088 => presumably real branches
- > 100 => presumably real branches
The fact that the code allows numbers and strings is more than a little strange. Of course, 10B and 10C can be considered hex numbers, but if the prefixes are all treated as hex numbers, 10B and 10C fall outside of the test range and would be treated as real branches.
This likely means that the prefix is stored as a string but treated as a number in some cases. Here is the simplest code I can think of that replicates this behavior (using C# for illustrative purposes):
bool IsTest(string strPrefix) {
int iPrefix;
if(int.TryParse(strPrefix, out iPrefix))
return iPrefix >= 89 && iPrefix <= 100;
return true; //here is the problem
}
In English, if the string is a number and is between 89 and 100, it's a test. If it's not a number, it's a test. Otherwise it's not a test.
If the code follows this pattern, no unit test would have caught this at the time the code was deployed. Here are some example unit tests:
assert.isFalse(IsTest("088"))
assert.isTrue(IsTest("089"))
assert.isTrue(IsTest("095"))
assert.isTrue(IsTest("100"))
assert.isFalse(IsTest("101"))
assert.isTrue(IsTest("10B")) // <--- business rule change
The unit test shows that "10B" should be treated as a test branch. User @gnasher729 above says that the business rules changed and that's what the last assertion above shows. At some point that assert should have switched to an isFalse, but that didn't happen. Unit tests get run at development- and build-time but then at no point afterwards.
What's the lesson here? The code needs some way to signal that it received unexpected input. Here is an alternative way to write this code that emphasizes that it expects the prefix to be a number:
// Alternative A
bool TryGetIsTest(string strPrefix, out bool isTest) {
int iPrefix;
if(int.TryParse(strPrefix, out iPrefix)) {
isTest = iPrefix >= 89 && iPrefix <= 100;
return true;
}
isTest = true; //this is just some value that won't be read
return false;
}
For those who don't know C#, the return value indicates whether or not the code was able to parse a prefix from the given string. If the return value is true, the calling code can use the isTest out variable to check if the branch prefix is a test prefix. If the return value is false, the calling code should report that the given prefix is not expected, and the isTest out variable is meaningless and should be ignored.
If you are ok with exceptions, you can do this instead:
// Alternative B
bool IsTest(string strPrefix) {
int iPrefix = int.Parse(strPrefix);
return iPrefix >= 89 && iPrefix <= 100;
}
This alternative is more straightforward. In this case, the calling code needs to catch the exception. In either case, the code should have some way of reporting to the caller that it didn't expect a strPrefix that couldn't be converted to an integer. In this way the code fails fast and the bank can quickly find the problem without the SEC fine embarrassment.
