Hi,
I just have a few minor follow-ups on code implementation (not approach, as this was not the general solution for the specific problem at hand. I mainly wanted to point out a few of the approaches to coding to keep in mind (especially as our existing codebase is full of legacy code and non-best-practices)
On Mon, May 26, 2014, at 09:01 AM, Tomasz Boczkowski wrote:
The tricky part can be the implementation of NRPaintServer::create method. I have thought about two solutions: a) using a battery of dynamic_cast if statements like this: NRPaintServer* NRPaintServer::create(SPPaintServer* ps) { SPPattern* pattern = dynamic_cast<SPPattern*>ps; if (pattern != 0) { return new NRPattern(&pattern); } //and so on for gradients }
In general I dislike overuse of dynamic_cast<>. Sometimes it's helpful, but in other cases exposing an enum on the base class to denote the 'general type' of the specific subclass can help. Among other things it makes the expected behavior a little more visible, allows for use in switch statements, executes faster, etc. In this context I think it would not be a good solution, however.
b) using double-dispatch strategy. There would be an .cpp file internal class NRPaintServerFactory. class NRPaintServerFactory { public: virtual NRPaintServer* create(SPPaintServer*); virtual NRPaintServer* create(SPLinearGradient*); virtual NRPaintServer* create(SPRadialGradient*); //and so on }
That API is very "C-ish". A more C++ approach is to avoid pointers, add const-correctness, etc.
class NRPaintServerFactory {
public:
virtual NRPaintServer* create(SPPaintServer const &server) const;
virtual NRPaintServer* create(SPLinearGradient const &server) const;
virtual NRPaintServer* create(SPRadialGradient const &server) const;
//and so on
Notice also that I did include the name for the parameter, even though technically it is not required. Good names help add to maintainability of code.
But again, this is just using your example code to have concrete instances to discuss parameters, etc. As Krzysztof has mentioned, a different higher-level approach should be used in this specific case.
-- Jon A. Cruz jon@...18...