Refactoring poor Design: Strategy-Pattern?

[mad_ferret]

I'm currently working with a system/data hierarchy designed as you can see below. Systems represent bare interfaces to some sort of hardware component which is able to deliver raw data to then be further processed into some final representation.

class SystemData
{
}

class SystemDataA : public SystemData
{
  int x;
}

class SystemDataB : public SystemData
{
  float y;
}

class System
{
  virtual SystemData* getData() = 0;
  virtual Result computeData(SystemData*) = 0;
}

class SystemA : public System
{
  // really returns SystemDataA
  SystemData* getData() override;
  Result computeData(SystemData*) override;
}

class SystemB : public System
{
  // really returns SystemDataB
  SystemData* getData() override;
  Result computeData(SystemData*) override;
}

void Controller::finalProcessing()
{
  for(auto& s : systemVec)
  {
    SystemData* data = s->getData();
    FinalResult final = s->computeData(data);
  }
}

The problem here is: Each system returns a concrete data type abstracted away in some common interface, which is a nice idea. To further process it, there is however still some sort of runtime information needed from which system the data came from because the further processing steps depend on this. To get to this piece of information, the author apparently just passed it back into the system where imo it doesn't belong at all, dynamic_casts to the concrete type in the first line of each compute-function and does the processing there. To me this looks like circumventing all the nice purposes of abstract inheritance hierarchies whilst making it overly complicated.

So what I'm thinking of to make this better is some sort of encapsulated processing algorithm hierarchy. Since they don't carry any state and are really just single methods, maybe a map of function pointers will already do. Each data type then has to carry a piece of information (either just an enum type or a function pointer) of which algorithm to use but can in the end be done completely isolated from the system.
The problem remains however that I must dynamic_cast or identify the concrete data type to feed it into the correct algorithm. So this still doesn't look like the cleanest design there is. Any ideas on how to make this better?

[tiliavirga]

The problem remains however that I must dynamic_cast or identify the concrete data type to feed it into the correct algorithm. So this still doesn't look like the cleanest design there is. Any ideas on how to make this better?

The abstraction to SystemData obviously doesn't work so I wouldn't hesitate to downcast. It's often the easiest way to restore type information that has been carelessly and prematurely abstracted away. The problem with downcasting is that it may fail at runtime but if you're 100% sure it will succeed the problem is only hypothetical. It will never materialise as a bug.

Otherwise the standard OO way of getting rid of downcasting is the Visitor pattern. In this case it will include changes to all subclasses of SystemData (since they will need an accept() method to reveal what subtype of SystemData they are). Considering this complication it may be better to simply just downcast or even better still, to re-design from ground up steering clear of downcasting altogether.

[superbonzo]

if each System is expected to computeData() only with its own getData(), then computeData() should take no argument and work on its own known, internal data directly.

if each System is expected to computeData() on <any> SystemData, then visitor is the way to go, but keep in mind that visitor is much more rigid than it appears, so it deserves special care ...

if each System is expected to computeData() on <any> SystemData, but it's also expected to take special actions when computing its own getData() ( as it seems from your code snippet ), then there's no need of downcasting, you can just either

class SystemA : public System
{
  SystemData* getData() override{ return data_; }
  Result computeData(SystemData* sd) override
  {
    if( sd == data_ ) { /*my data*/ }
    else { /*general data*/ }
  }
private:
  SystemDataA* data_;
}

or let getData() return covariantly, if you need more flexibility down the hierarchy ... :

class SystemA : public System
{
  SystemDataA* getData() override; // covariant override
  Result computeData(SystemData* sd) override
  {
    if( sd == getData() ) { /*my data*/ }
    else { /*general data*/ }
  }
}

[mad_ferret]

The reason we moved the computation from inside the system to outside of it is that we wanted to have a network interface to our systems and transmitting lightweight raw data was way simpler than transmitting the final data type with all sorts of metadata. Given this in mind, I think the disadvantages of moving it back into the system overweight.

The visitor pattern looks like an ok solution thus far. What I don't like about it though is that the computation function of each data subtype has to be declared in the visitor base class.
What if I just implement a hierarchy of computation algorithms outside of the system and add a field std::function<RetVal(Params)> compFunc inside the data classes which stores a pointer to the concrete function to use? In the data base class I can then just implement a method:

RetVal compute(Params p)
{
return compFunc(p);
}

and do this at the controller's side:

void Controller::finalProcessing()
{
for(auto& s : systemVec)
{
SystemData* data = s->getData();
FinalResult final = data->compute();
}
}