tags:

views:

218

answers:

10

I am updating an old piece of C++ code and am stuck on a design issue and need advice on the best course of action. The code handles geometric data. Currently, the code defines many global constants to handle element types:

#define TETRAHEDRON 0
#define HEXAHEDRON 1

Each constant has information associated with it that remains constant and which is currently handled by a class, in our case Topology.

int Topology::nodesPerElement(int topType)
{    
    switch(topType) {
        case TETRAHEDRON:
            return 4;
            break;
        case HEXAHEDRON:
            return 8;
            break;
    }
}

The Topology class has many of these functions that simply switch on the global constant to figure out associated information. There are a lot of element types and many bugs are introduced by switch statements that don't consider all element types. If an element type is added all of these methods need to be fixed. I need a better way of doing this that keeps the associated information with the type.

Enumerations are an improvement over this design, but it doesn't solve the problem of associating data with the enumeration.

For simplicity, I would like to avoid needing to instantiate classes for each type, as each will contain only static data that doesn't change.

What I really need is a "static class" that holds this information and performs like the pseudocode below:

class Tetrahedron : public TopType {
    static const int nodesPerElement = 4;
    static const std::string name = "Tet";
    etc...
}

Each method in Topology becomes trivial:

int Topology::nodesPerElement(TopType topType)
{
    return topType.nodesPerElement;
}

Is there a way to do this in C++? I've thought about just getting rid of the enumerations and having separate child Topology classes for each TopologyType, but the feedback I get from others is that it's too complicated of a solution. I hope that my question is clear enough.

+2  A: 

I'm not sure who advised you to avoid derived classes for each Toplogy type. To my eye, this problem is screaming for derived classes.

Unless you would need a very large number of such classes.

Christopher Bruns
Well, if there are a million topology types, would you want a million classes?
Moron
-1 I concur with Moron. Writing countless subclasses for trivial properties that are not really runtime behavior is a bad smell, IMO.
Emile Cormier
Ouch. OK. I see that my design sense needs some more refinement. Thanks for the feedback.
Christopher Bruns
Undid -1 in light of your revision. :-) Note that the OP says: "Currently, the code defines many global constants to handle element types".
Emile Cormier
+1  A: 

Personally I think the best way to store this information would be to create a general Shape class. Then, instead of coding all those static variables put them in a file/database and load your shape information from the data store when you start your program.

Pace
+1  A: 

Couldn't you use a record to do this if your goal is to avoid class instantiation?

Really though, you should class the poop out of this.

Jeremy Petzold
+2  A: 

You can have classes with nothing but static member variables. And that's a nice way to encapsulate attribute data.

If you'd rather not do that, traits might get you what you want.

John Dibling
Compile-time traits will not be useful if the topology is only known at runtime. Perhaps the OP would like to load shapes from a file.
Emile Cormier
+1  A: 

If topType is contiguous and starting a 0, you could just maintain an array of structs and index into that, instead of trying to have classes and subclasses. This way the only code change you would need is to

  • add the struct: Easy
  • add an array of structs: Easy
  • change each method to index into array and return proper field of struct: Tedious, but you have to do this anyway.

It your TopologyType can just be modelled as an instance of a struct (i.e no methods on it etc), Classes + Derived classes is overkill, IMO.

Moron
A: 

This is precisely what virtual functions are for. The classical way to do it would be:

class Topology 
{
  public:

  virtual int nodesPerElement() const = 0;
  // etc
};

class Tetrahedrom : public Topology
{
   public:
   virtual nodesPerElement() const { return 4; }
   // etc
}

// etc

But if you really have an aversion to re-implementing the accessor methods (as opposed to just defining variables) you could do the following with templates (although it's really no less verbose):

class Topology 
{
  public:

  virtual int nodesPerElement() const = 0;
  // etc
};

template<typename T>
class ConcreteTopology : public Topology
{
  public:

  virtual int nodesPerElement() const { return T::nodesPerElement; }
  // etc
};

struct Tetrahedron_Data {
  int nodesPerElement = 4; 
  // etc
};

typedef ConcreteTypology<Tetraheadron_Data> Tetrahedron;

// etc
Tyler McHenry
+8  A: 

Create a base class that contains all of the properties that your objects should support, and a private constructor to set those properties. You don't need derived classes, then: you can use static public objects to create the objects that you want with the desired properties.

class TopologyObject
{
    private:
        int numberVertices;
        int numberFaces;
        // etc.

    public:
        int getVertices() { return numberVertices; };
        int getFaces() { return numberFaces; };

    protected:
        TopologyObject(int vertices, int faces) :
            numberVertices(vertices),
            numberFaces(faces)
        {};

    public:
        static TopologyObject Tetrahedron = new TopologyObject(4, 4);
        // etc.
}

You can access the Tetrahedron with all of its properties via TopologyObject::Tetrahedron.

If you decide that you need more complex variable behavior based on the type of object, then you really do need derived classes and virtual methods for the overrideable behavior.

JSBangs
This looks like the best of both worlds. It becomes easy to define and add properties to each TopologyObject, but it avoids the need to have derived classes for each Topology type. I agree that if more complex variable behavior was needed derived classes and virtual methods would be a must, but this behavior is all I need for my application, and this works beautifully.Thanks for all the ideas after several days banging my head on my desk attempting various implementations!
devillighter
+1 This is the "Replace Subclass with Fields" refactoring: http://www.refactoring.com/catalog/replaceSubclassWithFields.html I hate it when people abuse polymorphism for things that are not really "behavior".
Emile Cormier
The `static Tetrahedron = new TopologyObject(4, 4)` is not valid C++. It looks like a Java-ism to me. :-)
Emile Cormier
@Emile, it's not a Java-ism either, it's a brain fart. Fixed.
JSBangs
Hate to nitpick, but that's still not valid C++. First, `new TopologyObject(4, 4)` returns a pointer. Second, only static const integral type members can be initialized directly in the class declaration like that. For other types, you need to use member initializer lists in the constructor definition. Anyways, don't sweat the syntax. We get the gist of what you're trying to do.
Emile Cormier
+3  A: 

Unless your Topology types have different runtime behaviors (like drawing themselves), then I agree with your peers that sub-classing is overkill. Reporting static properties like nodesPerElement and name is hardly a runtime behavior.

Unless you are not telling us the whole story about Topology, it seems that what you need is a simple property map. Use std::map to associate a topology type code with a structure of topology properties. This refactoring resembles Replace Subclass with Fields.

Here's some code that may serve as inspiration:

#include <cassert>
#include <iostream>
#include <map>
#include <string>

struct Topology
{
    enum Code {tetrahedron, hexahedron};
    int nodesPerElement;
    std::string name;
};

namespace // Anonymous namespace
{
    // Lookup table associating topology code with properties
    const struct {Topology::Code code; Topology topo;} topoTable_[] =
    {
        {Topology::tetrahedron,  {4, "Tetrahedron"}},
        {Topology::hexahedron,   {6, "Hexahedron"}}
    };
};

class TopologyMap // Singleton
{
public:
    static TopologyMap lookup(Topology::Code code)
    {
        return Topology(instance().doLookup(code));
    }

private:
    typedef std::map<Topology::Code, Topology> Map;
    Map map_;

    TopologyMap()
    {
        // Initialize map with constant property table
        size_t tableSize = sizeof(topoTable_) / sizeof(topoTable_[0]);
        for (size_t row=0; row<tableSize; ++row)
        {
            map_[topoTable_[row].code] = topoTable_[row].topo;
        }
    }

    static TopologyMap& instance()
    {
        static TopologyMap instance;
        return instance;
    }

    const Topology& doLookup(Topology::Code code) const
    {
        Map::const_iterator match = map_.find(code);
        assert(match != map_.end());
        return match->second;
    }
};

class Shape
{
public:
    Shape(Topology::Code topoCode)
    : topo_(TopologyMap::lookup(topoCode)) {}

    const Topology& topology() const {return topo_;}
    // etc...

private:
    Topology topo_;
};

int main()
{
    Shape shape1(Topology::tetrahedron);
    Shape shape2(Topology::hexahedron);
    std::cout << "shape1 is a " << shape1.topology().name << " with " <<
        shape1.topology().nodesPerElement << " nodes per element.\n";
    std::cout << "shape2 is a " << shape2.topology().name << " with " <<
        shape2.topology().nodesPerElement << " nodes per element.\n";
};

Output:

shape1 is a Tetrahedron with 4 nodes per element.
shape2 is a Hexahedron with 6 nodes per element.

If the topology code is zero-based and continuous, then you may use simple array indexing instead of a map. However, array indexing will be more error-prone if someone messes around with the topology code enum. Here is the same example that uses array indexing:

#include <cassert>
#include <iostream>
#include <map>
#include <string>

struct Topology
{
    enum Code {tetrahedron, hexahedron, CODE_COUNT};
    int nodesPerElement;
    std::string name;
};

namespace // Anonymous namespace
{
    // Lookup table associating topology code with properties
    const Topology topoTable_[] =
    {
        {4, "Tetrahedron"},
        {6, "Hexahedron"}
    };
};

class TopologyMap // Singleton
{
public:
    static Topology lookup(Topology::Code code)
    {
        assert(code < Topology::CODE_COUNT);
        return topoTable_[code];
    }

private:
    TopologyMap() {} // Non-instantiable
};

class Shape
{
public:
    Shape(Topology::Code topoCode)
    : topo_(TopologyMap::lookup(topoCode)) {}

    const Topology& topology() const {return topo_;}
    // etc...

private:
    Topology topo_;
};

int main()
{
    Shape shape1(Topology::tetrahedron);
    Shape shape2(Topology::hexahedron);
    std::cout << "shape1 is a " << shape1.topology().name << " with " <<
        shape1.topology().nodesPerElement << " nodes per element.\n";
    std::cout << "shape2 is a " << shape2.topology().name << " with " <<
        shape2.topology().nodesPerElement << " nodes per element.\n";
};

Note that because the details of storing and retrieving Topology was encapsulated in TopologyMap, I didn't have to rewrite any code in Shape and main.

Emile Cormier
+1  A: 

Since (apparently) all the relevant data is available at compile time, one possibility would be to use an enumeration along with templates and specialization to do the job:

enum { tetrahedron, hexahedron };

template <int type>
struct nodes_per_element { int operator()() const { 
    throw std::invalid_argument("Attempt to use unknown shape");
};

template <>
struct nodes_per_element<tetrahedron> { int operator()() const { return 4; } };

template <>
struct nodes_per_element<hexahedron> { int operator()() const { return 8; } };

You'd use this like: int x = nodes_per_element<hexahedron>()(); If you try to use it for a value for which there's no specialization, that will invoke the un-specialized template, which will throw an exception, halting the program and (normally) displaying a message saying you attempted to use an unknown shape. Of course, you can customize how that's displayed (if at all).

This should quickly show where you have problems due to values that haven't been defined.

The other obvious possibility would be to just define a struct for each shape you're going to use, and create an array of those structs, using the name of the shape as an index into the data, and the name of the specific data you want will be the member of the struct. For just the nodes per element you've given, that would look like:

struct shape_data { 
    int nodes_per_element;
    std::string name;
};

shape_data data[] = { 
    {4, "Tetrahedron"}, 
    {8, "Hexahedron" }
};

Retrieving data would be something like:

shape_data &s = data[hexahedron];

std::cout << "A " << s.name << " has " << s.nodes_per_element << "nodes per element.\n";
Jerry Coffin
Your first alternative will not be useful if the topology is only known at runtime.
Emile Cormier
For your second alternative, I would also provide a "factory" function (or singleton) that returns a shape_data when given a shape enum. This interface would hide how the shape_data is stored and retrieved. If it is later decided that the shape_data is stored in a map, then the change would not affect client code using the factory interface.
Emile Cormier
@Emile: Under the circumstances, it seems more like a "repository" than a factory (i.e. it's only returning existing data, not creating anything new) but that might become a good addition at some point. I don't think there's any particular urgency to doing so, however -- if you did, the obvious interface would be to overload `operator[]`, so you could put it in place without breaking any client code.
Jerry Coffin
@Jerry: Good point. I was thinking of the wrong metaphor.
Emile Cormier
@Emile:Yup -- happens to all of us. Good idea, but the wrong word for it popped out...
Jerry Coffin
+1  A: 

Having look at the previous answers, I've decided to add my own.

To me there are 2 things that I would require of such a design:

  • the ability to define a new item without recompiling the whole program
  • the ability to look up an item based on a property (like the number of faces)

This can be quite easy to do, so here is my little bit of code:

class Solid
{
  typedef std::vector<Solid> solids_type;
public:
  Solid(std::string name, size_t faces, size_t nodes):
    mName(name), mFaces(faces), mNodes(nodes)
  {
  }

  ///
  /// Properties
  ///
  const std::string& getName() const { return mName; }
  size_t getFaces() const { return mFaces; }
  size_t getNodes() const { return mNodes; }

  ///
  /// Collection Handling
  ///
  static bool Add(Solid solid); // only add if it's not already there.

  ///
  /// struct Predicate: std::unary_function<Solid,bool>
  ///
  template <class Predicate>
  static const Solid* Get(Predicate pred)
  {
    solids_type::const_iterator it =
        std::find_if(Solids().begin(), Solids().end(), pred);
    return it == Solids().end()) ? 0 : &(*it);
  } // Get

  ///
  /// Some Predicates
  ///
  class ByName: std::unary_function<Solid,bool>
  {
  public:
    ByName(std::string name): mName(name) {}
    bool operator()(const Solid& s) const { return s.getName() == mName; }
  private:
    std::string mName;
  };

  class ByFaces; /// ...
  class ByNodes; /// ...

private:
  /// Properties
  std::string mName;
  size_t mFaces;
  size_t mNodes;

  /// Collection
  static solids_type& Solids()
  { 
    static solids_type MSolids;
    return MSolids;
  }
}; // class Solid

And thus, now we can have:

// in tetrahedron.cpp
namespace
{
  bool gTetrahedron = Solid::Add(Solid("Tetrahedron", 4, 4));
}

// in main.cpp
int main(int argc, char* argv[])
{
  const Solid* myTetra = Solid::Get(Solid::ByFaces(4));

  assert(myTetra->getName() == "Tetrahedron");
  assert(myTetra->getFaces() == 4);
  assert(myTetra->getNodes() == 4);

  return 0;
} // main

And now we have met our goals:

  • Adding one new solid does not cause any recompilation
  • We can lookup solid based on their properties

We could also imagine:

  • being able to iterate through all the registered solids
  • having them sorted by number of faces, or whatever
  • defining a little macro for the registration
Matthieu M.
+1 This also allows the loading of solid types at runtime from a file. BTW, you might be interested in using Boost.MultiIndex to do the ByName, ByFaces, etc...
Emile Cormier
I usually like `Boost`, but in this case, I'd rather not. It is difficult to know beforehand which predicate may interest the user, so it's easier to let him compose his own at leisure.
Matthieu M.