Below you can see how the basic method looks like.
As you can see, there are several hardcoded literals in the method. For me, it was difficult to understand the algorithm for PIN block format 1 (a.k.a ISO-1) encoding is supposed to be applied, just from the reading this method. To understand it better I had to look at the actual definition of how should the PIN block format 1 be implemented. This is, in my opinion, the first issue with the method above – you have to move away from the code to be able to understand what and how the code works. For some complex things it’s always good to consult the available documentation, to make sure you understand the flow. On the other hand, for something simple such as PIN block encoding, I think the code should be written well enough that you don’t have to look further.
- Initialize the byte array of length 8 as the resulting pin block – OK, that sounds clear enough.
- Convert string “14” to packed BCD and add to the result value, at the first position – it’s really not clear why “14″ is used here, but let’s move on and see what we’ll find.
- Convert plain PIN to packed BCD – clear.
- Copy packed BCD representation bytes to the result value, one by one – this also looks a bit off, but it’s not clear what the issue is at the first reading of the method.
- Create a temporary variable with prefix ‘rnd’, suggesting random, with not so random value “7743438284” – this is really smelly, but ok, we’ll see.
Without knowing exactly the algorithm to create PIN block format 1, it’s not clear what exactly should be done to create it, just by reading this method.
Except for hardcoded values, one more problem that I find with the method above, is that every now and then I have to switch my thoughts and think about what does the method stringToPackedBcd do. I think that it is breaking the reading flow. Creation of the PIN block is interrupted each time by conversion to BCT. You can see in the refactored solution that my proposal is to encode PIN to PIN block and only after you are done with it, convert it to BCD, in a single step.
Below you can find the implementation of the stringToPackedBcd method.
Can you spot the bug(s), after reading these two methods?
You can visit this page to make sure you understand the PIN block format 1 encoding.
- Hardcoded value “14” – first part is fine, since it represents the PIN block format version, but the value 4 represents the PIN length. It should be deducted from the actual PIN length, not set to 4 always.
- PIN conversion to BCD and copying to the result value – it actually copies only the first four digits from the BCD encoded PIN, not the complete value.
- Random filler values are not random at all, but rather a hardcoded string value.
- The last one is the issue that caused a bug report – if the value passed to the BCD encoding method is not of even length, an exception will be thrown.
If you take a better look you will see that the method stringToPackedBcd declares one parameter, which is incorrectly named pin. Only in one call to this method the actual PIN is passed as an argument, in all other cases, it is just a numeric value that needs conversion.
Since the code had so many issues, I decided to rewrite it, and to make it more readable. Below you can see the new version of the method.
As you can see, the method now has a flow that clearly depicts the algorithm for PIN block format 1 encoding. Reading the method is straightforward and the conversion to BCD is performed only once, after the complete PIN block is created.
Also, you can see that the code is much more readable when you use constants with meaningful names, instead of the hardcoded values. You may know what it represents at the moment of writing, but you will most probably forget the meaning after some time, when you come back to the code.
You can find the complete refactoring case in my GitHub repository.
I’ll be happy to hear your thoughts and ideas for further improvements.
Superb, what a weblog it is! This weblog gives useful information to us,
keep it up.