0

While working with some code base, I am trying to understand piece of code so as can work and customize it , I am able to understand almost 90% of the code flow. Here is the overall flow

  1. Code is being used to generate 15 digit code (alphanumeric), first 3 digits are customer provided.
  2. Initially code is generating 16 digit alphanumeric number and storing it in the cache.
  3. Customer can generated any number of code by specifying quantity.
  4. all customer generated codes are being generated from the 16 digit number (point 2). All code generated have numbers/ alphabets from that 16 digit alphanumeric number.
  5. When some one try to use those codes, system is trying to validate if the provided code is valid or not.

I am struck at the logic used to determine if the provided code is valid or not, here is that piece of code, I am generating 6 code as a sample, in this case alphanumeric code being generated and stored in the cache is

 initial-alphabet : M9W6K3TENDGSFAL4

Code generated based on initial-alphabet are myList=[123-MK93-ES6D-36F3, 123-MK93-EFTW-D3LG, 123-MK93-EALK-TGLD, 123-MK93-ELKK-DN6S, 123-MK93-E4D9-3A6T, 123-MK93-EMTW-LNME]

 protected  int getVoucherNumber(String voucherCode){
  int voucherNumberPos = voucherCode.length() - 12;
  String voucherNumberHex = voucherCode.substring(voucherNumberPos, voucherNumberPos + 6);
  int firstByte = getIntFromHexByte(voucherNumberHex.substring(0, 2), 0);
  int secondByte = getIntFromHexByte(voucherNumberHex.substring(2, 4), 1);
  int thirdByte = getIntFromHexByte(voucherNumberHex.substring(4, 6), 7);
  return firstByte << 16 | secondByte << 8 | thirdByte;
}

private int getIntFromHexByte(String value, int offset){
  return (getIntFromHexNibble(value.charAt(0), offset) << 4) + getIntFromHexNibble(value.charAt(1), offset + 4);
} 

private int getIntFromHexNibble(char value, int offset){
  int pos = getAlphabet().indexOf(value);
  if (pos == -1) {// nothing found}
    pos -= offset;
  while (pos < 0) {
    pos += 16;
  }
    return pos % 16;
 }

Here is the code which is trying to validate code

 int voucherNumber = getVoucherNumber(kyList.get(4));

In this case value of voucherNumber is 4 i.e the fourth element from the list, in case I pass any value which is not part of the list getVoucherNumber method is returning a higher value (greater than the list count).

One of the main thing which confusing me are these 2 lines

int voucherNumberPos = voucherCode.length() - 12;
String voucherNumberHex = voucherCode.substring(voucherNumberPos, voucherNumberPos + 6);

As per my understanding, they are first moving out the first 3 digit from the check which are customer supplied but again they have not used rest of the string but only specific part of the string.

Can any one help me to understand this

2
  • 2
    Why is this tagged as java8? Is there something specific about Java 8 in this code sample? I don't see it, but maybe I missed it... Commented Oct 8, 2014 at 18:38
  • @FrustratedWithFormsDesigner: Nothing related to java-8, I tagged it by mistake, thanks for correcting Commented Oct 9, 2014 at 1:58

1 Answer 1

5

It appears you've inherited responsibility for some poorly written code. We've all been there so I'll try to answer in that spirit. I'm not positive this question is on-topic for this site, but it doesn't appear to be forbidden by the help center. In an attempt to stay on-topic I'll end with some general advice not limited to the highly-localized specifics of the question.

myList.get(4)

Arrays in Java are zero-based, so that's 123-MK93-E4D9-3A6T. You probably know that, but it isn't clear from your question that you do.

initial-alphabet : M9W6K3TENDGSFAL4

I assume this is what's returned by the call to getAlphabet in getIntFromHexNibble. So the alphanumeric characters in the code are meant to be hexadecimal but using a nonstandard set of 16 characters for the digits.

protected  int getVoucherNumber(String voucherCode){

Ignoring the hyphens and the customer-supplied first three digits, the code is 'MK93E4D93A6T'. Twelve hex digits encode 48 bits, but an int in Java is only 32 bits long, so the code is already broken. Whatever it does, it isn't going to return the voucher number represented by the voucher code.

int voucherNumberPos = voucherCode.length() - 12;
String voucherNumberHex = voucherCode.substring(voucherNumberPos, voucherNumberPos + 6);

This is setting voucherNumberHex to a six-character long string, starting twelve from the end of voucherCode, in this case 93-E4D. It seems likely the author didn't expect the caller to include the hyphens when this code was first written. Even so the intent seems to be to ignore half the voucher code.

int firstByte = getIntFromHexByte(voucherNumberHex.substring(0, 2), 0);
int secondByte = getIntFromHexByte(voucherNumberHex.substring(2, 4), 1);
int thirdByte = getIntFromHexByte(voucherNumberHex.substring(4, 6), 7);

This looks straightforward at first, but the parameters 0, 1, and 7 are not offsets at all, despite the name of the argument. It's trying to turn each pair of hex digits into a byte, which would be sensible enough if not for the hyphen character. Now for the fun part:

private int getIntFromHexNibble(char value, int offset) {
    int pos = getAlphabet().indexOf(value);
    if (pos == -1) {// nothing found}
        pos -= offset;

        while (pos < 0) {
            pos += 16;
        }
        return pos % 16;
    }

The right curly brace after "found" is commented out, so the code you posted is actually incomplete. I'm going to assume there's another line or two that read

    return pos;
}

So the basic idea is that M becomes 0, 9 becomes 1, and so on via the call to indexOf. But if this method sees a character not in the provided alphabet, like a hyphen, it uses the so-called offset to calculate a default value (in this case 14, if I've done the math in my head right), and returns that as the hex nibble value.

The end result is that you get back a number in the range 0 (inclusive) to 2^24 (exclusive). But of the 2^24 possible values such a number should have, only 2^20 different values will ever be returned. So from a voucher code that looks like twelve digits of base-32, which would have an astronomical number of values, you're limited to slightly over a million different voucher numbers within each customer prefix.

General advice:

  • Use peer reviews to prevent code like this from getting into production.
  • Use unit tests to prove the code does what the function name says it does.
  • Use exceptions to fail early if the input isn't what you're expecting.
Sign up to request clarification or add additional context in comments.

4 Comments

Peer reviews are only useful if they don't devolve into navel examinations, which is what they become over time, almost without exception. Pairing is better IMO -- it's a better balance of "sharpening the ax" and "getting stuff done" than most peer reviews. One brain can solve a problem, two can solve it well, three is only marginally better, and by the time you get to four brains, you're mostly just wasting money and time.
@JoeRounceville I agree having more than one or two people review a given piece of code tends to be unproductive. My organization does that only for knowledge transfer or to demonstrate a prototyped approach. Peer reviews as we do them are when each person's code is reviewed by one or two of their peers.
@gatkin: Thanks for the wonderful reply, unfortunately I can not change this code :( . if (pos == -1) {}, here an exception is being returned.What I am confused is about the part of voucher code being used for validation as the code did not took complete voucher code but only specific part of it. Do you think there is any logic behind this?
@UmeshAwasthi Obviously my answer is not going to say anything about Java code that wasn't in your question. None of the code you included does any validation; it just extracts a number encoded (in a convoluted way) in the larger string. If I'd designed it (not knowing any of your requirements) the voucher code would be base 32, not disguised hex, with some of the bits being the voucher number and the rest being an HMAC to validate the authenticity of the voucher number. (I'm assuming it has financial value so you don't want people to make up their own.)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.