current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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";
}
share|improve this question

3 Answers 3

up vote 2 down vote accepted

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;
    }
}
share|improve this answer
up vote 2 down vote

  • 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().

share|improve this answer
up vote 2 down vote

  • 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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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