Algorithm with byte shifts and BCD calculation

Question

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:

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?

Answer 1

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.

Answer 2

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();