2
\$\begingroup\$

I am writing a program that will take a binary bar code and convert it to a zip-code. I need to validate the bar code. A valid binary bar code contains no more and no less than two 1s in any position per five digits. This is what I have for the validation part so far and would like simplify this code.

#include <iostream>
#include <string>

using std::cout;
using std::string;

int main()
{
    const int SIZE = 5;//number of characters to test in a set
    string bar = "100101010010001";

    string tempstring;
    int count = 0;
    while (count < bar.length())
    {
        int test = 0;
        tempstring = bar.substr(count, SIZE);//split bar string into next five characters

        for (int index = 0; index < SIZE; index++)
        {
            if (tempstring[index] == '1')
            {
                ++test;
            }
        }

        if (test != 2)
        {
            cout << "Invalid bar code!\n";
            exit(1);
        }

        count += 5;
    }

    cout << "Valid bar code!\n";
}
\$\endgroup\$
2
\$\begingroup\$

You can do without creating a temp string, simply by using bar[count + index] instead of tempstring[index]. Also, the while loop would be more natural as a for loop. Like this, with some other improvements:

for (int count = 0; count < bar.length(); count += 5)
{
    int ones = 0;

    for (int index = 0; index < SIZE; index++)
    {
        if (bar[count + index] == '1')
        {
            ++ones;
        }
    }

    if (ones != 2)
    {
        cout << "Invalid bar code!\n";
        return 1;
    }
}
\$\endgroup\$
2
\$\begingroup\$
  • The constant SIZE and its comment don't quite seem consistent. A name such as size would correspond to an actual size of, say, a structure. But with such a comment, it doesn't seem to be for that. Consider renaming the constant to something more accurate.

  • bar should be const since it's not modified.

  • test seems like an ambiguous name for an integer variable. It sounds more like a Boolean test flag, but that's not the case here. Consider renaming it to something more accurate.

  • Since tempstring is only used within the while loop, it can just be initialized there. It doesn't need to be declared before it.

  • You shouldn't mismatch integer types when doing comparisons:

    while (count < bar.length())
    

    Since length() returns an std::size_t, which is an unsigned type, have count be of type std::size_t as well. If you had compiler warnings up, then this should've been reported.

  • This may be a small nit-pick, but you can just return 1 instead of calling exit() in main().

\$\endgroup\$
2
\$\begingroup\$
  • Last substring may contain less than SIZE characters. I suppose you want to reject such strings too.

  • string.substr() creates a new object. It doesn't matter in a toy program; in general I recommend to avoid creating temporary objects.

  • You may want to consider std::accumulate() instead of spelling out the counting loop.

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