2

At several places here in SO, it is claimed that having to know the exact type of an object and make a decision based on that (in an if-then-else manner) points to a design flaw, e.g. here.

I am wondering if this is always the case. In a current small educational project (which I'm using to learn C++) I want to implement arithmetic structures, i.e. terms (such as (3+4)*x-5) and equations (x=3*y+z). Now, from a structural point of view, equations are very similar to terms (particularly they can all be parsed using the Shaunting-yard algorithm), so I created a base class math_struct that both equation and term derive from:

class math_struct;
class term : public math_struct {...};
class equation : public math_struct {
    void resolve_to(std::string subTerm);
    ...
};

equation has a method resolve_to that term does not have (since an equation can be resolved to a variable, which a term cannot), but other than that they are equal.

Now I have a method to parse a math_struct from a string:

static std::unique_ptr<math_struct> parse_math_struct(std::string formula);

The resulting math_struct can either be a term or an equation, depending on what the input string looks like. To know which it is, I have to do a typeid check and perform a cast to use, if available, the resolve_to member function.

Is this a design flaw – and how could I improve it?

tambre
  • 4,625
  • 4
  • 42
  • 55
Duke
  • 410
  • 7
  • 15
  • 1
    Is `math_struct` an empty class? – Rakete1111 Jun 18 '18 at 07:16
  • `resolve_to(std::string)` doesn't return anything? – YSC Jun 18 '18 at 07:18
  • `math_struct` is not empty, it provides common functionality such as `to_string()`. `resolve_to` does not return anything, but restructures the equation such that some variable is on one side and the rest on the other. E.g., 3*x+4 = 0 ==> resolve_to("x") ==> x = -4/3 – Duke Jun 18 '18 at 07:25
  • 3
    Show some [MCVE]. It is not always a design flow, but it could be one – Basile Starynkevitch Jun 18 '18 at 07:31
  • A think that a `dynamic_cast` could be enough here. – Serge Ballesta Jun 18 '18 at 07:44
  • Even though this example is somewhat incomplete, the upcasting here definitely seems to be a design flow. Either base class lacks the necessary methods or downcasting is performed unreasonably. So you can either add all the required methods into the base class or make `parse_math_struct` behave like a dispatcher, rather than factory, that is return `void` but accept two callbacks to be invoked when `term` or `equation` object is constructed. – user7860670 Jun 18 '18 at 08:05
  • 1
    The types have nothing in common beyond being parseable with the same algorithm. I would keep the classes unrelated and probably have the parser return a [`std::variant`](http://en.cppreference.com/w/cpp/utility/variant) – molbdnilo Jun 18 '18 at 08:26
  • Seems to me as if terms could well have a `resolve_to()` which is a NOP. – Peter - Reinstate Monica Jun 18 '18 at 09:12

2 Answers2

1

I don't know if I would call it a "flaw", but I would say the design could be improved. One potential problem with checking the type to see if you can call resolve_to is that the check could break if, for some reason, you find the need for a third class derived from math_struct. (Someone with a mathematical background should be aware that being unable to think of a reason why does not mean there is no reason why.)

A more robust option is to add a virtual function to the base class, a simple Boolean that returns true iff you can call resolve_to.

An even better option might be to think about what do you do if the thing you're dealing with is not an equation.

To know which it is, I have to do a typeid check and perform a cast to use, if available, the resolve_to member function.

What do you do if the resolve_to member function is not available? Why not have a virtual function on the base class that is overridden to do that thing for terms and overridden to call resolve_to for equations? (Or that is resolve_to for equations.)

There might be a situation where the best option is to "ask which exact type an object has", but I do not believe this is it. (I know there's an XML parser that relies on this technique; I have not analyzed that to see if it could be improved.)

JaMiT
  • 14,422
  • 4
  • 15
  • 31
1

You're not going to be able to write a generic resolve_to. You'll need a few different variants, for different types of equations. Yes, it's simple for a linear_equation. It's not too hard for a quadratic_equation. Third- and 4th-order polynomials can still be resolved, and that's it - provable.

Either way - the fact that you need a few resolve_to variants suggests that you want a few classes derived from equation, and all you need to know is whether an object IS-A equation. The exact type is pointless.

MSalters
  • 173,980
  • 10
  • 155
  • 350
  • You're right in principle (although I'm not planning on implementing all these sophisticated `resolve_to`-methods ;-) ). I agree (also with the other comments) that only being "parseable" in the same way might not justify having the same base class. But still I find it hard to avoid double code in this context in a logical way without running into this "design flaw". – Duke Jun 18 '18 at 15:16