2
\$\begingroup\$

I'm trying to develop an algorithm that generates a 16-digit validation number to interact with a given system.

This is how the algorithm should be implemented:

Validation Algorithm

I have implemented it, and have it working with 2 of 3 test cases that I have found (I will post them below).

The implementations goes as:

public static String Generate(byte[] _MachineId, byte[] _SN)
{
        try
        {
            byte[] step1 = new byte[6] 
            {
                _SN[2], 
                _SN[1], 
                _SN[0], 
                _MachineId[2],
                _MachineId[1],
                _MachineId[0] 
            };

            byte[] step2 = new byte[6]
            { 
                step1[0],
                step1[1],
                (byte)(step1[2] ^ step1[0]),
                (byte)(step1[3] ^ step1[1]),
                (byte)(step1[4] ^ step1[0]),
                (byte)(step1[5] ^ step1[1])
            };

            var crc45 = new byte[2] { step2[4], step2[5] }.GetCRCAsByteArray(0);
            var crc23 = new byte[2] { step2[2], step2[3] }.GetCRCAsByteArray(0);
            var crc01 = new byte[2] { step2[0], step2[1] }.GetCRCAsByteArray(0);

            byte[] step3 = new byte[6]
            {
                crc01[0],
                crc01[1],
                crc23[0],
                crc23[1],
                crc45[0],
                crc45[1]
            };


            string bin_to_BCD_543 = ((uint)((((0x00 << 4) | step3[5]) << 16) | ((step3[4] << 8) | step3[3]))).ToString().PadLeft(8, '0');
            string bin_to_BCD_210 = ((uint)((((0x00 << 4) | step3[2]) << 16) | ((step3[1] << 8) | step3[0]))).ToString().PadLeft(8, '0');

            String step4pre = bin_to_BCD_543 + bin_to_BCD_210;
            Func<String, int[]> convertToIntArray =
                    new Func<string, int[]>(
                    (str) =>
                    {
                        List<int> result = new List<int>();
                        foreach (var c in str.ToCharArray()) result.Add(int.Parse(c.ToString()));
                        return result.ToArray();
                    }
                );
            int[] step4 = convertToIntArray(step4pre);

            int[] step5 = step4.ToList().ToArray();

            step5[8] = step4[8] | ((step4.ToList().GetRange(8, 8).Sum() % 5) << 1);
            step5[0] = step4[0] | ((step4.ToList().GetRange(0, 8).Sum() % 5) << 1);

            String validation = String.Join("", step5);
            return validation;
        }
        catch (Exception exc)
        {
            System.Diagnostics.Trace.TraceError("Error generating Validation number. Error => " + exc.ToString());
        }
        return "";
    }

This, are the 2 test cases that I have working:

ID = {0x21, 0x43, 0x65}, SN = {0x01, 0x00, 0x00} -> VN = "6429188185446104"
ID = {0x21, 0x43, 0x65}, SN = {0x02, 0x00, 0x00} -> VN = "2701773123879856"

I have found a 3rd case in another document, that should satisfy this:

ID = {0x01, 0x00, 0x10}, SN = {0x99, 0x00, 0x00} -> VN = "0230633823748784"

But this one does not works for me, so my questions are:

  • Is the algorithm implementation OK in terms of satisfying the requirements?
  • How can this implementation be improved?
\$\endgroup\$

migrated from stackoverflow.com Apr 11 '12 at 15:10

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ What does yours return for the last case? \$\endgroup\$ – Joshua Drake Apr 11 '12 at 15:12
  • \$\begingroup\$ I would say that 3rd case example is faulty since your algorith diagram and example is from official SAS documentation. \$\endgroup\$ – user76025 Jun 18 '15 at 11:15
3
\$\begingroup\$

The algorithm seems to be fine, and if it does not fail the tests it should be OK (try to get more test cases though).

Regarding the implementation:

  1. Indentation - not sure if it just the site or you deliberately placed your curly braces the way they are placed, but it they certainly look weird, especially with the array initialization. You should indent when there is a new block, no matter what.

  2. Use var in place of explicit types - makes reading and refactoring easier.

  3. Argument names - In the C# world, the _variable naming conventionally means a private member variable. Even if you choose not to honor that convention, your argument names are very confusing. Instead of 1-2 letter abbreviations you should use descriptive names.

  4. Too long - split into multiple methods. It doesn't matter that they are only called once: descriptive function names will give you a "bird's view" of the algorithm. Also, you should not use Func just because you can; it's cool, but code is better when not cool but clear, so turn that into a normal method.

  5. Explain the padding part in a comment - it is actually a nice solution, but a bit counter-intuitive (my first thought was: why would you convert to string to just convert back?, but it's actually cleaner than filling the padding zeros by hand).

  6. step4.ToList().ToArray() - is a confusing (and half as fast) way of saying step4.Clone(). BTW it wouldn't hurt to just use Lists and avoid the other ToList calls as well.

  7. I would argue that you don't need the try-catch block around the whole method, since the only thing that looks like it could throw is your CRC calls, so you should only handle that part. If anything else throws, it's better if it's ugly and noisy (a.k.a. don't catch it; clients who call your method will get the empty string back and no other indication of failure, which is not really a good practie.

  8. Use XML documentation comments for the method to document purpose, return value, arguments.

  9. Make sure that a public static method is appropriate for your needs. Instead of public, you might want to use internal. And remember, you can't override static methods in derived classes, so you can't take advantage of polymorphism later.

\$\endgroup\$
  • \$\begingroup\$ 1. the indentation was broken when it was posted, I will try to fix it here. 7. the whole method is surrounded by try/catch, as this method is only used by us, and on a module that handles money among other things, and although I like what u suggest, in this case it's better that the method does not work, than the program crashing somewhere else! \$\endgroup\$ – miguel.hpsgaming Apr 11 '12 at 16:48
  • 1
    \$\begingroup\$ When the program crashes somewhere else, you will have an exception object that contains the call stack. If that's still not acceptable, you should surround the call to this method and handle specialized Exception classes. Handling exceptions the way you do does not prevent them, it just makes them invisible. If something weird happens, like you run out of memory, you would not want to continue running the program anyway. The best you can do is fail gracefully. \$\endgroup\$ – Tamás Szelei Apr 11 '12 at 17:33
0
\$\begingroup\$

The following section of code can be reduced/simplified:

Func<String, int[]> convertToIntArray =
        new Func<string, int[]>(
        (str) =>
        {
            List<int> result = new List<int>();
            foreach (var c in str.ToCharArray()) result.Add(int.Parse(c.ToString()));
            return result.ToArray();
        }
    );
int[] step4 = convertToIntArray(step4pre);

First, I would change the body of the function to use LINQ. Then, since the function is only used in one place, I would in-line it:

var step4 = step4pre.ToCharArray().Select(c => int.Parse(c.ToString())).ToArray();

Next, if performance isn't a huge issue, I would drop the ToCharArray, as string implements IEnumerable<char> already:

var step4 = step4pre.Select(c => int.Parse(c.ToString())).ToArray();

Finally, as it turns out, there is a function called Char.GetNumericValue(char) which retrieves the numeric value of a character for you, so you could use the following alternative:

var step4 = step4pre.Select(c => (int) char.GetNumericValue(c)).ToArray();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.