3
\$\begingroup\$

Every coder knows that if you give six developers an assignment to write a bit of code that will perform some specific task, you will end up with at least six different ways of doing it.

I recently realized/discovered a radical example of this. I wrote some code to calculate check digits for barcodes. I then found some legacy code of ours to do the same thing. I have tested both with several barcodes, and so far they produce the same results, but I find it rather fascinating just how different the "how" of the solutions are.

If anybody wants to play around with it, I'll add the entire code below (you will see in the button click event what you will need to name the few labels and buttons and the edit control); but first: the two methods that calculate the check value (actually, my solution uses several functions, which are called by the primary function).

First, the legacy function:

public static char CalculateChkDigit(string CheckValue)
{
    if (CheckValue.Length > 6)
    {
        char ch;
        int a, b, i, j;
        a = 0;
        b = 0;
        j = ((int)CheckValue.Length) - 1;
        i = j;
        while (i >= 0)
        {
            a = a + (int)(CheckValue[i]) - 48;
            i = i - 2;
        }

        j = ((int)CheckValue.Length) - 2;
        i = j;
        while (i >= 0)
        {
            b = b + (int)(CheckValue[i]) - 48;
            i = i - 2;
        }
        a = 3 * a + b;
        b = a % 10;
        if (b != 0) b = 10 - b;

        ch = (char)(48 + b);
        return ch;
    }
    else
        return ' ';
}

Actually, this was cleaned up using Resharper to become this in my test/comparison utility:

private static char GetBarcodeChecksumWithLegacyCode(string barcodeWithoutCzechSum)
{
    if (barcodeWithoutCzechSum.Length > 6)
    {
        int a = 0;
        int b = 0;
        int j = barcodeWithoutCzechSum.Length - 1;
        int i = j;
        while (i >= 0)
        {
            a = a + barcodeWithoutCzechSum[i] - 48;
            i = i - 2;
        }

        j = barcodeWithoutCzechSum.Length - 2;
        i = j;
        while (i >= 0)
        {
            b = b + barcodeWithoutCzechSum[i] - 48;
            i = i - 2;
        }
        a = 3 * a + b;
        b = a % 10;
        if (b != 0) b = 10 - b;

        var ch = (char)(48 + b);
        return ch;
    }
    return ' ';
}

Now, here's my code:

public static string GetBarcodeChecksum(string barcode)
{
    int oddTotal;
    int oddTotalTripled;
    int evenTotal;
    // Which positions are odd or even depend on the length of the barcode, 
    // or more specifically, whether its length is odd or even, so:
    if (isStringOfEvenLen(barcode))
    {
        oddTotal = sumInsideOrdinals(barcode);
        oddTotalTripled = oddTotal * 3;
        evenTotal = sumOutsideOrdinals(barcode);
    }
    else
    {
        oddTotal = sumOutsideOrdinals(barcode);
        oddTotalTripled = oddTotal * 3;
        evenTotal = sumInsideOrdinals(barcode);
    }
    int finalTotal = oddTotalTripled + evenTotal;
    int modVal = finalTotal%10;
    int czechSum = 10 - modVal;
    if (czechSum == 10)
    {
        return "0";
    }
    return czechSum.ToString();
}

private static bool isStringOfEvenLen(string barcode)
{
    return (barcode.Length % 2 == 0);
}

// "EvenOrdinals" instead of "EvenVals" because values at index 0,2,4,etc. are seen by the 
// checkdigitmeisters as First, Third, Fifth, ... (etc.), not Zeroeth, Second, Fourth
private static int sumInsideOrdinals(string barcode)
{
    int cumulativeVal = 0;
    for (int i = barcode.Length-1; i > -1; i--)
    {
        if (i % 2 != 0)
        {
            cumulativeVal += Convert.ToInt16(barcode[i] - '0');
        }
    }
    return cumulativeVal;
}

// "OddOrdinals" instead of "OddVals" because values at index 1,3,5,etc. are seen by the 
// checkdigitmeisters as Second, Fourth, Sixth, ..., not First, Third, Fifth, ...
private static int sumOutsideOrdinals(string barcode)
{
    int cumulativeVal = 0;
    for (int i = barcode.Length - 1; i > -1; i--)
    {
        if (i % 2 == 0)
        {
            cumulativeVal += Convert.ToInt16(barcode[i] - '0');
        }
    }
    return cumulativeVal;
}

As is probably understandable, I prefer my code, because, although more verbose/less "elegant", it seems easier to grok to me AND -- the reason why N coders will come up with N+N solutions to a challenge -- I think differently than the other cat/come from a different "place" in approaching a solution to this challenge. As long as they both work, it's hard to say one way is better than the other; however, I do find it quite interesting just how different the two approaches are. The legacy code looks to me like something from "A Beautiful Mind" - impressive, but (for me, at least) practically impenetrable.

Finally, here's all the code, for the tinkerers in the audience:

using System;
using System.Windows.Forms;

namespace BarcodeCzechDigitTester
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            string barcodeWithoutCzechSum = textBox1.Text.Trim();
            // calculate check sum using my code
            string czechSum = GetBarcodeChecksum(barcodeWithoutCzechSum);
            string barcodeWithCzechSum = string.Format("{0}{1}", barcodeWithoutCzechSum, czechSum);
            label1.Text = barcodeWithCzechSum;
            // calculate check sum using legacy code (which I had been avoiding due to its seemingly overly complex logic)
            char checkSumLegacy = GetBarcodeChecksumWithLegacyCode(barcodeWithoutCzechSum);
            string barcodeWithCheckFromSumLegacyCode = string.Format("{0}{1}", barcodeWithoutCzechSum, checkSumLegacy);
            labelLegacy.Text = barcodeWithCheckFromSumLegacyCode;

            textBox1.Focus();
        }

        private static char GetBarcodeChecksumWithLegacyCode(string barcodeWithoutCzechSum)
        {
            if (barcodeWithoutCzechSum.Length > 6)
            {
                int a = 0;
                int b = 0;
                int j = barcodeWithoutCzechSum.Length - 1;
                int i = j;
                while (i >= 0)
                {
                    a = a + barcodeWithoutCzechSum[i] - 48;
                    i = i - 2;
                }

                j = barcodeWithoutCzechSum.Length - 2;
                i = j;
                while (i >= 0)
                {
                    b = b + barcodeWithoutCzechSum[i] - 48;
                    i = i - 2;
                }
                a = 3 * a + b;
                b = a % 10;
                if (b != 0) b = 10 - b;

                var ch = (char)(48 + b);
                return ch;
            }
            return ' ';
        }

        public static string GetBarcodeChecksum(string barcode)
        {
            int oddTotal;
            int oddTotalTripled;
            int evenTotal;
            // Which positions are odd or even depend on the length of the barcode, 
            // or more specifically, whether its length is odd or even, so:
            if (isStringOfEvenLen(barcode))
            {
                oddTotal = sumInsideOrdinals(barcode);
                oddTotalTripled = oddTotal * 3;
                evenTotal = sumOutsideOrdinals(barcode);
            }
            else
            {
                oddTotal = sumOutsideOrdinals(barcode);
                oddTotalTripled = oddTotal * 3;
                evenTotal = sumInsideOrdinals(barcode);
            }
            int finalTotal = oddTotalTripled + evenTotal;
            int modVal = finalTotal%10;
            int czechSum = 10 - modVal;
            if (czechSum == 10)
            {
                return "0";
            }
            return czechSum.ToString();
        }

        private static bool isStringOfEvenLen(string barcode)
        {
            return (barcode.Length % 2 == 0);
        }

        // "EvenOrdinals" instead of "EvenVals" because values at index 0,2,4,etc. are seen by the 
        // checkdigitmeisters as First, Third, Fifth, ... (etc.), not Zeroeth, Second, Fourth
        private static int sumInsideOrdinals(string barcode)
        {
            int cumulativeVal = 0;
            for (int i = barcode.Length-1; i > -1; i--)
            {
                if (i % 2 != 0)
                {
                    cumulativeVal += Convert.ToInt16(barcode[i] - '0');
                }
            }
            return cumulativeVal;
        }

        // "OddOrdinals" instead of "OddVals" because values at index 1,3,5,etc. are seen by the 
        // checkdigitmeisters as Second, Fourth, Sixth, ..., not First, Third, Fifth, ...
        private static int sumOutsideOrdinals(string barcode)
        {
            int cumulativeVal = 0;
            for (int i = barcode.Length - 1; i > -1; i--)
            {
                if (i % 2 == 0)
                {
                    cumulativeVal += Convert.ToInt16(barcode[i] - '0');
                }
            }
            return cumulativeVal;
        }

        private void button2_Click(object sender, EventArgs e)
        {
            string bcVal = textBox1.Text.Trim();
            bool validCzechDigit = isValidBarcodeWithCheckDigit(bcVal);
            MessageBox.Show(validCzechDigit ? string.Format("{0} is valid", bcVal) : string.Format("{0} invalid", bcVal));
        }

        private static bool isValidBarcodeWithCheckDigit(string barcodeWithCheckDigit)
        {
            string barcodeSansCheckDigit = barcodeWithCheckDigit.Substring(0, barcodeWithCheckDigit.Length - 1);
            string czechDigit = barcodeWithCheckDigit.Substring(barcodeWithCheckDigit.Length - 1, 1);
            //MessageBox.Show(string.Format("raw barcode portion is {0}", barcodeSansCheckDigit));
            //MessageBox.Show(string.Format("czech portion is {0}", czechDigit));
            return GetBarcodeChecksum(barcodeSansCheckDigit) == czechDigit;
        }

    }
}

To help visualize the GUI:

enter image description here

Any comments/observations?

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm happy everyone kept the CzechSum. \$\endgroup\$ – JohnMark13 Sep 5 '13 at 16:36
  • \$\begingroup\$ @JohnMark13 Well, barcodes were invented by the Czech mathematician Barbora Coděláš, that's why it's called Czech sum. :-) \$\endgroup\$ – svick Sep 5 '13 at 17:35
  • \$\begingroup\$ Love lateral thinking. \$\endgroup\$ – JohnMark13 Sep 5 '13 at 20:03
2
\$\begingroup\$

The first thing that caught my eye, was that they are not functionally equivalent. Yours is missing the case when a barcode is below 7 in lenght. Another thing that strikes me as odd about both solutions is the verbosity and inefficiency involved in both. Both needlessly iterate twice, and im not sure the names of variables and methods in your rewrite, are accurately portraying what they are actually doing. As you encouraged people to tinker with it, heres is my shot at it:

public static string Check(string barcode)
    {
        if (barcode.Length <= 6) return " ";

        var sums = new int[2];
        for (int i = 0; i < barcode.Length; i++)
        {
            sums[(i+1)%2] += barcode[i] - '0';
        }

        sums[barcode.Length%2] *= 3;

        int mod = sums.Sum()%10;
        return (mod == 0 ? mod : 10 - mod).ToString();
    }
\$\endgroup\$
  • \$\begingroup\$ Actually, mine is the one that does deal with any length of barcode, from 1 to 42Googleplex (give or take). Whether that matters or not (are there barcodes less than 7 in length?), I don't know. Thanks for your tinkering/tweaking. \$\endgroup\$ – B. Clay Shannon Sep 5 '13 at 19:16
  • 1
    \$\begingroup\$ As far as naming goes, I consider "Check" to be too vague. \$\endgroup\$ – B. Clay Shannon Sep 5 '13 at 19:17
  • \$\begingroup\$ Oh, I see what you're saying - mine *shouldn't" allow barcodes below 7. Okay, point taken. \$\endgroup\$ – B. Clay Shannon Sep 5 '13 at 19:17
  • \$\begingroup\$ Touché about the vague naming :) \$\endgroup\$ – Frank Sep 5 '13 at 19:19
  • 1
    \$\begingroup\$ And to be frank, Frank is kind of a frank name, too. \$\endgroup\$ – B. Clay Shannon Sep 5 '13 at 20:23
1
\$\begingroup\$

Caveat: I've never written a line of C#, nor do I know anything about its compiler.

I think that the approach taken in your code could be considered the more modern approach. It is definitely more suitable for human consumption, and it makes it more easy to test individual parts of the algorithm. With luck it will compile down to very similar byte code too.

But, your code is inelegant in that you loop twice skipping alternate elements. You also have a bit of smell where you have the code if (isStringOfEvenLen(barcode)). I'm not sure what the accepted usage of Tuples is in C# but for internal use I'd have thought a single function that returns you an (insides, outsides) would be acceptable - if not a little helper class with two elements. This would modify you code to begin (pseudo-ish):

Tuple<int, int> sums = sumOrdinals();
int finalTotal = generateTotal(sums);

Where generateTotal looks like this:

private static int generateTotal(Tuple<int, int> sums)
{

    if (isStringOfEvenLen(barcode)) {
        return (sums.Item1 * 3) + sums.Item2;
    }

    return (sums.Item2 * 3) + sums.Item1;
}

Then onwards. Arguably this is borderline procedural but with these few steps I think that is acceptable.

Edit: Less readable, more old school

public static string Check(string barcode)
{
    int sum = 0;
    for (int i = 0; i < barcode.Length; i++)
    {
        sum += i & 0x01 == 1 ? barcode[i] - '0' : (barcode[i] - '0') * 3;
    }

    int mod = sum % 10;
    return mod == 0 ? 0 : 10 - mod;
}

Now what you really want to do of course is collapse down that for loop. I found that if you have LINQ (which I see you do not) you can use the Aggregate operator, very simple e.g:

int sum = barcode.ToCharArray().Aggregate(0, (sum, value) => sum.Add(value - '0'));

So, if you're clever about getting an index in there you could write the algorithm (without validation) in just 3 lines and you'd have done a bit of functional programming which sets you up nicely for playing with a new language.

\$\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.