Should I use an enum or multiple const for non-sequential constants in c++?

Question

I'm writing porting file-io set of functions from c into a c++ class. "Magic numbers" (unnamed constants) abound.

The functions read a file header which has a number of specific entries whose locations are currently denoted by magic numbers.

I was taught by a veteran programmer a couple years back that using "magic numbers" is inherently evil, and thus, I have since tried to avoid using unnamed constants in my port. So I want to create some sort of list of constants of where the entries are stored.

So far I've come up with two solutions that seem relatively safe -- use a namespace enclosed set of constants or a namespace enclosed enum.

Can I use either solution safely? Would there be any advantages to one over the other?

e.g.
OPTION 1

namespace hdr_pos {
   const unsigned int item_1_pos=4;
   const unsigned int item_2_pos=8;
   const unsigned int item_3_pos=12;
   const unsigned int item_4_pos=24;
   const unsigned int item_5_pos=32;
};

OPTION 2

namespace hdr_pos {
   enum e {
      item_1_pos=4,
      item_2_pos=8,
      item_3_pos=12,
      item_4_pos=24,
      item_5_pos=32
   };
};

Is there anyway to prevent duplicates, to catch if I change the positions due to a future update to the file header, but forget to change one of them?

Please keep this factual and non-subjective. If there is no advantage you know of, feel free to answer that.

Note: I would use more descriptive names, of course, in my actual implementation; I just called things item_<#>_... for examples sake...

Answer 1

The title of your question suggests that the main reason you have doubts about using a enum is that your constants are non-iterative. But in C++ enum types are non-iterative already. You have to jump through quite a few hoops to make an iterative enum type.

I'd say that if your constants are related by nature, then enum is a pretty good idea, regardless of whether the constants are iterative or not. The main disadvantage of enums though is total lack of type control. In many cases you might prefer to have strict control over the types of your constant values (like have them unsigned) and that's something enum can't help you with (at least yet).

Answer 2

I can see two advantages to using an enum. First, some debuggers can translate constants back into enum variable names (which can make debugging easier in some cases). Also, you can declare a variable of an enumerated type which can only hold a value from that enumeration. This can give you an additional form of type checking that you wouldn't have simply by using constants.

Checking to see if a value is duplicated might depend on your particular compiler. The easiest way to do so would probably be to write an external script that will parse your enum definition and report whether or not a value is duplicated (you can run this as part of your build process if you like).

Answer 3

The issue here is that you'd like to define rules and you'd like them to work at compile time. Coming up with fancy coding schemes and various trickery in this case is not a good use of your time. Instead, what I'd have you do is use a static analyzer on your code which contains the rules you'd like to see obeyed by your code. Place this as part of the integration testing or with whatever continuous build tool you guys are using.

Be sure to have the analyzer check only that the values are not the same, do not replicate the values within the code and the rule list. Duplication would not serve you well.

Answer 4

One thing to keep in mind is that you can't take the address of an enum:

const unsigned* my_arbitrary_item = &item_1_pos;

Answer 5

If they're purely constants and require no run-time stuff (like can't init enum with non-enum value) then they should just be const unsigned ints. Of course, the enum is less typing, but that's besides the point.

Answer 6

I've dealt with this situation before, for error codes.

I have seen people using enums for error codes, and this pose some issues:

1. you can assign an int to the enum that doesn't not correspond to any value (too bad)
2. the value itself is declared in a header, meaning that error code reassignment (this happens...) breaks code compatibility, you also have to take care when adding elements...
3. you have to define all codes in the same header, even if often times some code are naturally restricted to a small portion of the application, because enums cannot be "extended"
4. there is no check that a same code is not assigned twice
5. you cannot iterate over the various fields of an enum

When designing my error codes solution, I thus chose another road: constants in a namespace, defined in source files, which address points 2 and 3. To gain in type safety though, the constants are not int, but a specific Code class:

namespace error { class Code; }

Then I can define several error files:

// error/common.hpp

namespace error
{
  extern Code const Unknown;
  extern Code const LostDatabaseConnection;
  extern Code const LostNASConnection;
}

// error/service1.hpp
// error/service2.hpp

I didn't solved the arbitrary cast issue though (constructor is explicit, but public), because in my case I was required to forward error codes returned by other servers, and I certainly didn't want to have to know them all (that would have been too brittle)

However I did thought about it, by making the required constructor private and enforcing the use of a builder, we're even going to get 4. and 5. in a swoop:

// error/code.hpp

namespace error
{
  class Code;

  template <size_t constant> Code const& Make(); // not defined here

  class Code: boost::totally_ordered<Code>
  {
  public:
    Code(): m(0) {} // Default Construction is useful, 0 is therefore invalid

    bool operator<(Code const& rhs) const { return m < rhs.m; }
    bool operator==(Code const& rhs) const { return m == rhs.m; }

  private:
    template <size_t> friend Code const& Make();
    explicit Code(size_t c): m(c) { assert(c && "Code - 0 means invalid"); }

    size_t m;
  };

  std::set<Code> const& Codes();
}


// error/privateheader.hpp (inaccessible to clients)

namespace error
{
  std::set<Code>& PrivateCodes() { static std::set<Code> Set; return Set; }

  std::set<Code> const& Codes() { return PrivateCodes(); }

  template <size_t constant>
  Code const& Make()
  {
    static std::pair< std::set<Code>::iterator, bool > r
      = PrivateCodes().insert(Code(constant));
    assert(r.second && "Make - same code redeclared");
    return *(r.first);
  }
}

//
// We use a macro trick to create a function whose name depends
// on the code therefore, if the same value is assigned twice, the
// linker should complain about two functions having the same name
// at the condition that both are located into the same namespace
//
#define MAKE_NEW_ERROR_CODE(name, value)         \
  Make<value>(); void _make_new_code_##value ();


// error/common.cpp

#include "error/common.hpp"

#include "privateheader.hpp"

namespace error
{
  Code const Unkown = MAKE_NEW_ERROR_CODE(1)
  /// ....
}

A tad more work (for the framework), and only link-time/run-time check of the same assignment check. Though it's easy to diagnose duplicates simply by scanning for the pattern MAKE_NEW_ERROR_CODE

Have fun!