9
\$\begingroup\$

This program accepts either a 27 digit binary bar code of type string, or a 5 digit integer zip code of type int. The program can return a binary bar code of type string from a 5 digit zip code of type int or vise versa. I would like to know if this is a good implementation of a class. What would you suggest? I am also looking for any suggestions on simplifying the loops.

PostNet.h

    /*
   POSTNET:

         Prior to 2009 the bar code on an envelope used by the U.S.Postal Service represented
      a five(or more) digit zip code using a format called POSTNET. The bar code consists of 
      long and short bars. For this program, we will represent the bar code as a string of 
      digits. The digit 1 represents a long bar, and the digit 0 represents a short bar.
      Therefore, a bar code would be represented in our program as follows:

         110100101000101011000010011

         The first and last digits of the bar code are always 1. Removing these leaves 25
      digits. If these 25 digits are split into groups of five digits each then we have the
      following:

         10100 10100 01010 11000 01001

         Next, consider each group of five digits. There always will be exactly two 1’s in each
      group of digits. Each digit stands for a number. From left to right, the digits encode
      the values 7, 4, 2, 1, and 0. Multiply the corresponding value with the digit and 
      compute the sum to get the final encoded digit for the zip code. The following table 
      shows the encoding for 10100.

         Bar Code Digits:         1 0 1 0 0
         Value:               7 4 2 1 0
         Product of           
         Digit * Value:       7 0 2 0 0

         Zip Code Digit:          7+0+2+0+0 = 9

         Repeat this for each group of five digits and concatenate to get the complete zip code.
      There is one special value. If the sum of a group of five digits is 11, then this 
      represents the digit 0 (this is necessary because with two digits per group it is not
      possible to represent zero). The zip code for the sample bar code decodes to 99504. While
      the POSTNET scheme may seem unnecessarily complex, its design allows machines to detect 
      if errors have been made in scanning the zip code.

         Write a zip code class that encodes and decodes five-digit bar codes used by the U.S.
      Postal Service on envelopes. The class should have two constructors. The first constructor
      should input the zip code as an integer, and the second constructor should input the zip 
      code as a bar code string consisting of 0’s and 1’s as described above. Although you have
      two ways to input the zip code, internally, the class should only store the zip code using
      one format. (You may choose to store it as a bar code string or as a zip code number.) The
      class also should have at least two public member functions: one to return the zip code as
      an integer and the other to return the zip code in bar code format as a string. All helper
      functions should be declared private. Embed your class definition in a suitable test 
      program.
*/
#ifndef POSTNET_H
#define POSTNET_H

#include <iostream>
#include <string>

class PostNet
{
public:

   PostNet() = default;
   PostNet(int);
   PostNet(std::string);

   //must be a 27 character string of binary digits. The first and last digits of the
   //bar code are always 1. For every five digits there must be exactly two "1" digits
   void setBarCode(std::string); 
   //must be a 5 digit zip code >= 10000
   void setBarCode(int);
   //returns a 5 digit integer zip code
   int getZipCode() const;
   //returns a 27 character string of binary digits
   std::string getBarCode() const;

private:
   //validate bar code format
   bool validateBar(std::string) const;
   //convert integer zip code to string bar code
   std::string zipToBar(int) const;
   //convert string bar code to five digit zip code
   int barToZip(std::string) const;

   std::string barcode;
};
#endif //POSTNET_H

PostNet.cpp

#include <iostream>
#include "PostNet.h"

PostNet::PostNet(std::string bar)
{
   setBarCode(bar);
}
PostNet::PostNet(int zip)
{
   setBarCode(zip);
}
void PostNet::setBarCode(std::string bar)
{
   if (validateBar(bar))
      barcode = bar; 
   else
      exit(1);
}
void PostNet::setBarCode(int zip)
{
   std::string bar = zipToBar(zip);
   if (validateBar(bar))
      barcode = bar;
   else
      exit(1);
}
std::string PostNet::getBarCode() const
{
   if (validateBar(barcode))
      return barcode;
   else
      exit(1);  
}
int PostNet::getZipCode() const
{
   if (validateBar(barcode))
      barToZip(barcode);
   else
      exit(1);
}
//private member function to validate bar code format
bool PostNet::validateBar(std::string bar) const
{
   //if length of bar code is not 27 exit
   if (bar.length() != 27)
   {
      std::cout << "Invalid bar code!\n";
      return false;
   }

   const short GROUP_S = 5;//number of binary digits per group

   //test for exactly two 1's in each group of five digits. count set to 1 to 
   //skip index 0 of string, and length set to one less to skip last digit of string
   for (short count = 1; count < bar.length() - 1; count += GROUP_S)
   {
      short ones = 0;
      for (short index = 0; index < GROUP_S; index++)
      {
         if (bar[count + index] == '1')
            ++ones;
      }
      if (ones != 2)//if set of five binary digits does not contain exactly two "1" digits
      {
         std::cout << "Invalid bar code!\n";
         return false;
      }
   }

   return true;
}
// private function to convert 5 digit integer zip code to 27 digit binary string
std::string PostNet::zipToBar(int zip) const
{
   const short GROUP_S = 5;//number of binary digits per group
   const short ENCODE[] = { 7, 4, 2, 1, 0 };

   std::string bar;
   short zipdigit = 0;
   while (zip > 0)
   {
      std::string group = "00000";//group of five binary digits
      zipdigit = zip % 10;//zipdigit will hold integer from zip code starting in reverse order
      zip = zip / 10;//zip is truncated to have one less integer

      for (short i = 0; i < GROUP_S; i++)
      {
         for (short u = i + 1; u < GROUP_S; u++)
         {
            if (zipdigit == 0 && ENCODE[i] + ENCODE[u] == 11)
            {
                group[i] = '1';
                group[u] = '1';
                bar.insert(0, group);
            }
            if (zipdigit == ENCODE[i] + ENCODE[u])
            {
                group[i] = '1';
                group[u] = '1';
                bar.insert(0, group);
            }
         }
      }
   }

   bar.insert(0, "1");
   bar.insert(bar.length(), "1");

   return bar;
}
//private member function 27 character string to five digit integer zip code
int PostNet::barToZip(std::string bar) const
{
   const short GROUP_S = 5;//number of binary digits per group
   const short ENCODE[] = { 7, 4, 2, 1, 0 };

   std::string strzip;
   short count = 1;
   while (count < (barcode.length() - 1))
   {
      int mult = 0;
      for (short i = 0; i < GROUP_S; i++)
      {
         mult += (barcode[count++] - 48) * ENCODE[i];

         if (mult == 11)
            mult = 0;
      }
      strzip += std::to_string(mult);
   }

   int zip = atoi(strzip.c_str());
   return zip;
}
\$\endgroup\$
  • \$\begingroup\$ Is there a specific reason why you use integers for postal codes? I assume you are not doing math with them. ref1 ref2 \$\endgroup\$ – user31517 Jun 1 '15 at 13:59
  • \$\begingroup\$ That is a good question. Thinking about it now, it almost seems that it would have been easier to simply deal with the string type. However, from my understanding of the instructions for the problem, I made the five digit integer zip code of type int. Re-reading the instructions, I now see that it never explicitly stated for the zip to be of type int. I may go back and change zip code to a string. \$\endgroup\$ – Ritchie Shatter Jun 1 '15 at 14:58
12
\$\begingroup\$

Bad comments

   //validate bar code format
   bool validateBar(std::string) const;
   //convert integer zip code to string bar code
   std::string zipToBar(int) const;
   //convert string bar code to five digit zip code
   int barToZip(std::string) const;

Don't think any of those comments are actually required. Bad comments are worse than no comments. Your comments should explain WHY the code should explain how and be self documenting.

The problem with too many comments is that the code and comments will diverge over time. If you them come across code that does not behave like the comments describe which is correct? Do you fix the comments to match the code or do you fix the code to match the comments. If you describe WHY you need the information and leave the code to describe how then you do not fall into this maintenance trap.

Prefer throw to exit

void PostNet::setBarCode(std::string bar)
{
   if (validateBar(bar))
      barcode = bar; 
   else
      exit(1);
}
void PostNet::setBarCode(int zip)
{
   std::string bar = zipToBar(zip);
   if (validateBar(bar))
      barcode = bar;
   else
      exit(1);
}

It's a problem. Throw an exception. If the code does not explicitly try and handle the exception then the program will exit (so same result). But if you want to explicitly handle the error, you can now do so.

No need to test when you get

std::string PostNet::getBarCode() const
{
   if (validateBar(barcode))
      return barcode;
   else
      exit(1);  
}
int PostNet::getZipCode() const
{
   if (validateBar(barcode))
      barToZip(barcode);
   else
      exit(1);
}

You have validated that the bar code is valid when you set it. So there is no need to validate it when you get it. Just return the value.

Getters/Setters

Getters and setters are bad programming practice. They expose internal representation of your code. They also are rarely needed in well designed code. But there are situations when you need them. But you have not provided enough context on how the value will be used so I can not say it is bad (just probably bad).

Mutable

Your class has the perfect place where a mutable object can be used.

A mutable object is not part of the state of the object. But can be used to store a cached value. Here your state is held in the bar code. But you could also store the zip value in a mutable (initially it is zero to mark it as not set). But the first time you call getZip() you calculate it and store the value in a mutable member. Subsequent calls then just retrieve the zip from the stored value rather than re-calculating on each call.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestions. Is it ok if I resubmit this program after making the improvements offered by the code review community so that I may gain further suggestions? If so, would the program be submitted as a new question, or an extension of the existing question? \$\endgroup\$ – Ritchie Shatter Jun 2 '15 at 14:49
  • \$\begingroup\$ @RitchieShatter: That's totally fine. Just start a new question. Leave this one intact. \$\endgroup\$ – Martin York Jun 2 '15 at 17:05
10
\$\begingroup\$

Rethink the constructors

One of these constructors is not as good as the others:

PostNet() = default;
PostNet(int);
PostNet(std::string);

The parameterless one will create an object that's useless, as it won't have a barcode.

It's best to create objects that are valid and ready to use. That way you can trust every single instance, and know that they are valid. And since your class never changes the zip code or the bar code, it would be better to make these values immutable. (See my suggested implementation below.)

Bug in barToZip

The method receives std::string bar parameter, but then it works with barcode of the instance. This is unnecessary, confusing and error prone. It should use the received parameter, not the member variable.

Some functions can be static

validateBarCode, zipToBar and barToZip don't need instance data, so they can be static.

Make the class immutable

Incorporating the ideas above, and some other improvements, I suggest this implementation using immutable fields and helper static factory methods.

#include <iostream>
#include <string>
#include <stdexcept>

class PostNet
{
public:
   //returns a 5 digit integer zip code
   int getZipCode() const;
   //returns a 27 character string of binary digits
   std::string getBarCode() const;

   static PostNet fromZipCode(int);
   static PostNet fromBarCode(std::string);

private:
   PostNet(std::string, int);

   const std::string barcode;
   const int zipcode;

   //validate bar code format
   static bool isValidBarCode(std::string);

   //convert integer zip code to string bar code
   static std::string zipToBar(int);
   //convert string bar code to five digit zip code
   static int barToZip(std::string);
};

PostNet::PostNet(std::string barcode, int zipcode) : barcode(barcode), zipcode(zipcode) {}

PostNet PostNet::fromZipCode(int zip)
{
    std::string barcode = zipToBar(zip);
    if (isValidBarCode(barcode))
    {
        return PostNet(barcode, zip);
    }
    throw std::invalid_argument("invalid zip code");
}

PostNet PostNet::fromBarCode(std::string barcode)
{
    if (isValidBarCode(barcode))
    {
        return PostNet(barcode, barToZip(barcode));
    }
    throw std::invalid_argument("invalid bar code");
}

std::string PostNet::getBarCode() const
{
    return barcode;
}

int PostNet::getZipCode() const
{
    return zipcode;
}

bool PostNet::isValidBarCode(std::string barcode)
{
   //if length of bar code is not 27 exit
   if (barcode.length() != 27)
   {
      std::cout << "Invalid bar code!\n";
      return false;
   }

   const short GROUP_S = 5;//number of binary digits per group

   //test for exactly two 1's in each group of five digits. count set to 1 to 
   //skip index 0 of string, and length set to one less to skip last digit of string
   for (short count = 1; count < barcode.length() - 1; count += GROUP_S)
   {
      short ones = 0;
      for (short index = 0; index < GROUP_S; index++)
      {
         if (barcode[count + index] == '1')
            ++ones;
      }
      if (ones != 2)//if set of five binary digits does not contain exactly two "1" digits
      {
         std::cout << "Invalid bar code!\n";
         return false;
      }
   }

   return true;
}

std::string PostNet::zipToBar(int zipcode)
{
   const short GROUP_S = 5;//number of binary digits per group
   const short ENCODE[] = { 7, 4, 2, 1, 0 };

   std::string barcode;
   short zipdigit = 0;
   while (zipcode > 0)
   {
      std::string group = "00000";//group of five binary digits
      zipdigit = zipcode % 10;//zipdigit will hold integer from zip code starting in reverse order
      zipcode = zipcode / 10;//zip is truncated to have one less integer

      for (short i = 0; i < GROUP_S; i++)
      {
         for (short u = i + 1; u < GROUP_S; u++)
         {
            if (zipdigit == 0 && ENCODE[i] + ENCODE[u] == 11)
            {
                group[i] = '1';
                group[u] = '1';
                barcode.insert(0, group);
            }
            if (zipdigit == ENCODE[i] + ENCODE[u])
            {
                group[i] = '1';
                group[u] = '1';
                barcode.insert(0, group);
            }
         }
      }
   }

   barcode.insert(0, "1");
   barcode.insert(barcode.length(), "1");

   return barcode;
}
//private member function 27 character string to five digit integer zip code
int PostNet::barToZip(std::string barcode)
{
   const short GROUP_S = 5;//number of binary digits per group
   const short ENCODE[] = { 7, 4, 2, 1, 0 };

   std::string strzip;
   short count = 1;
   while (count < (barcode.length() - 1))
   {
      int mult = 0;
      for (short i = 0; i < GROUP_S; i++)
      {
         mult += (barcode[count++] - 48) * ENCODE[i];

         if (mult == 11) mult = 0;
      }
      strzip += std::to_string(mult);
   }

   return atoi(strzip.c_str());
}

To create PostNet instances, you can use the factory methods fromZipCode or fromBarCode, like this:

PostNet p1 = PostNet::fromZipCode(99999);
PostNet p2 = PostNet::fromBarCode("110100101001010010100101001");

If a valid instance cannot be constructed, an exception will be thrown.

\$\endgroup\$
6
\$\begingroup\$

In addition to what @Loki Astari said, I noticed the following things:

Error Handling

Your validation of the bar code is incomplete. In validateBar() you don't ensure that the first and last digits are 1. So a caller could pass in "0 01010 01010 01010 01010 01010 0" and it would validate, even though it's not a valid barcode.

Your conversion of int barcodes in setBarCode() will encode more than 5 digits, which will eventually get validated by validateBar(), but you could catch it earlier by just ensuring that zip <= 99999.

Header

Why don't you name the arguments in your header? It makes the intent much clearer. And if you did, you probably wouldn't need the comment for the int version.

\$\endgroup\$
3
\$\begingroup\$

Explicit Constructors

void f(PostNet pn) {}

int g() { f(5); }

Provided your header was included and a main was present that called g, would you expect this to compile? If not, your PostNet(int) constructor needs to be explicit PostNet(int). C++ will attempt to make up to one implicit conversion in the course of satisfying parameter types. This can be useful in certain situations, but anytime you have a class with a single parameter constructor, your brain should immediately jump to "I should make that explicit."

Only after a long argument with yourself should you even consider an implicit constructor, and even then, it should likely be rethought again.

std::bitset

I have a feeling std::bitset may prove useful to you. It is used to represent a fixed sized sequence of bits, and it provides functionality like converting to a string, converting from a string, and counting the number of high bits.

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