2

For example, I want to convert json from server to generic object: Shape, as follow:

{
    "type":"Circle",
    "radius":"3",
},
{
    "type":"Rectangle",
    "width":"5",
    "height":"6",
},

using c++ and rapidJson as example here:

/*main.cpp*/
void getResponse(const char* responseText){
    Document d;
    d.parse(responseText);
    for(SizeType i = 0; i < a.Size(); i++){
      if(d[i]["type"]="Circle"){
        this->shapeVector.push_back(new Circle(d[i]));
      }else if(...){
      }
    }

    for(Shape* s : this->shapeVector){
      printf("%f\n",s->getArea());
    }
}

I know it is very bad to use 

if(d[i]["type"]="Circle"){
}else if(...){
}

to convert a value to type here, so I try modify it to use registry pattern:

Shape:

/*Shape.h*/
#include <map>
class Shape{
public:
  virtual Shape* jsonToObj(const char* str)=0;
  virtual float getArea()=0; 
  static std::map<const char*,Shape*> objMap;
};
/*Shape.cpp*/
#include "Shape.h"
std::map<const char*,Shape*> Shape::objMap;

Circle:

/*Circle.h*/
#include "Shape.h"
class Circle : public Shape{
public:
  float radius;
  virtual Shape* jsonToObj(Value value){
    Circle* s=new Circle();
    s->radius=value["radius"];
    return s;
  }

  virtual float getArea(){
    return 3.14*this->radius*this->radius;
  }
  static long _;
};
/*Circle.cpp*/
#include "Circle.h"
long Circle::_=[](){ Shape::objMap["Circle"]=new Circle();return 0;}();

main:

/*main.cpp*/
void getResponse(const char* responseText){
    Document d;
    d.parse(responseText);
    for(SizeType i = 0; i < a.Size(); i++){
      this->shapeVector.push_back(Shape::objMap[d[i][name]].jsonToObj(d[i]));
    }

    for(Shape* s : this->shapeVector){
      printf("%f\n",s->getArea());
    }
}

which I convert value "type" to corresponding object type by asking Shape::objMap. When I need to add a type ,for example:Rectangle, I can add Rectangle.h and Rectangle.cpp without modify the if-else in main.cpp:

Rectangle:

/*Rectangle.h*/
#include "Shape.h"
class Rectangle : public Shape{
public:
  float width;
  float height;
  virtual Shape* jsonToObj(Value value){
    Rectangle* s=new Rectangle();
    s->width=value["width"];
    s->height=value["height"];
    return s;
  }

  virtual float getArea(){
    return this->width*this->height;
  }
  static long _;
};
/*Rectangle.cpp*/
#include "Rectangle.h"
long Rectangle::_=[](){ Shape::objMap["Rectangle"]=new Rectangle();return 0;}();

/*main.cpp*/
void getResponse(const char* responseText){
    Document d;
    d.parse(responseText);
    for(SizeType i = 0; i < a.Size(); i++){
      this->shapeVector.push_back(Shape::objMap[d[i][name]].jsonToObj(d[i]));
    }

    for(Shape* s : this->shapeVector){
      printf("%f\n",s->getArea());
    }
}

But according to Why is Global State so Evil?, I should not use global state, which

Shape::objMap

is a global map. My question is, is it a reasonable case to use global state? If-else and global map, which is more "evil" here?

aacceeggiikk
  • 717
  • 1
    Shouldn't it be a ShapeFactory instead of a Shape? If objMap["Circle"] is a Circle then it has a radius, which doesn't make sense. – user253751 Dec 03 '19 at 12:40

2 Answers2

3

The reason why global state is considered evil is because global state often introduces an invisible coupling between different parts of the software. This is especially problematic with mutable global state, because in a program of any size it becomes nearly impossible to track who changed that global state.

Read-only or "append-only" global state is less problematic, because you don't have the problem of unexpected changes coming from everywhere.

With a few small changes, you can make Shape::objMap effectively "append-only" memory in that new shapes can be registered, but existing registrations can't be overwritten.

/*Shape.h*/
#include <map>
class Shape{
public:
  virtual Shape* jsonToObj(const char* str)=0;
  virtual float getArea()=0; 

  static Shape* createShape(const char* name, const char* str)
  {
    if (objMap[name])
      return objMap[name]->jsonToObj(str);
    else
      throw "Requesting unregistered shape";
  }
  static void registerShape(const char* name, Shape* prototype)
  {
    if (objMap[name] == NULL)
        objMap[name] = prototype;
    else
      throw "Shape already registered";
  }
private:
  static std::map<const char*,Shape*> objMap;
};
/*Shape.cpp*/
#include "Shape.h"
std::map<const char*,Shape*> Shape::objMap;

My question is, is it a reasonable case to use global state? If-else and global map, which is more "evil" here?

The registry pattern, especially when implemented such that registry entries can't be overwritten is a reasonable use case for global state. On the other hand, the answer by @VectorZita nicely explains why the if-else ladder isn't evil either.

In the end, it comes down to

  • Is dynamic registration desirable/required? That isn't possible with the if-else ladder
  • Do you want compile-time checking that all relevant Shape classes are present? That is not possible with the Registry pattern.
  • What do you/your team like better?
Bart van Ingen Schenau
  • 74,630