3

Edit 2: TL;DR: Is there a way not to break OO best practices while still satisfying the constraint that a bunch of things of the same kind have to be convertible to a canonical thing of that kind?

Also, please keep in mind that my question is about the general situation, not the specific example. This isn't a homework problem.


Suppose you have the following:

  • an abstract base class that implements common functionality;
  • a concrete derived class that serves as the canonical representation.

Now suppose you want any inheritor of the base class to be convertible to the canonical representation. One way of doing this is by having an abstract method in the base class that is meant to return a conversion of the inheritor as an instance of the canonical derived class.

However, it seems to be generally accepted that base classes should not know about any of their derived classes, and in the general case, I agree. However, in this scenario, this seems to be the best solution because it enables any number of derived classes, each with their own implementation we don't need to know anything about, to be interoperable via the conversion to the canonical representation that every derived class has to implement.

Would you do it differently? Why and how?

An example for geometric points:

// an abstract point has no coordinate system information, so the values
// of X and Y are meaningless
public abstract class AbstractPoint {
    public int X;
    public int Y;

    public abstract ScreenPoint ToScreenPoint();
}

// a point in the predefined screen coordinate system; the meaning of X 
// and Y is known
public class ScreenPoint : AbstractPoint {
    public ScreenPoint(int x, int y) {
        X = x;
        Y = y;
    }

    public override ScreenPoint ToScreenPoint()
        => new ScreenPoint(X, Y);
}

// there can be any number of classes like this; we don't know anything
// about their coordinate systems and we don't care as long as we can
// convert them to `ScreenPoint`s
public class ArbitraryPoint : AbstractPoint {
    private int arbitraryTransformation;

    public ArbitraryPoint(int x, int y) {
        X = x;
        Y = y;
    }

    public override ScreenPoint ToScreenPoint()
        => new ScreenPoint(X * arbitraryTransformation, Y * arbitraryTransformation);

    // (other code)
}

Edit 1: The reason AbstractPoint and ScreenPoint are not the same class is semantic. An AbstractPoint does not have a defined coordinate system, therefore the values of X and Y in an AbstractPoint instance are meaningless. A ScreenPoint does have a defined coordinate system, therefore the values of X and Y in a ScreenPoint instance have a well-defined meaning.

If ScreenPoint were the base class, then an ArbitraryPoint would be a ScreenPoint, which is not the case. An ArbitraryPoint can be converted to a ScreenPoint, but that does not mean that it is-a ScreenPoint.

If you are still unconvinced, consider that an arbitrary coordinate system ACS1 can be defined as having a dynamic offset to the screen coordinate system SCS. This means the mapping between the two coordinate systems can vary with time, i.e. the point ACS1 (1, 1) can map to SCS (10, 10) at one moment, and SCS (42, 877) at another moment.

  • Is there a specific reason why a static class that handles this type of conversion wouldn't be an acceptable solution for your use case? However, at this time I am failing to see what your proposed `ScreenPoint` class brings to the table, that could not be instead placed on `AbstractPoint`. Can you expand upon this? – Chris Thompson Oct 28 '17 at 03:37
  • If everything is convertible to `ScreenPoint` it means, that `ScreenPoint` contains **only** data common to all classes. That suggests that `AbstractPoint` and `ScreenPoint` should be one class. – Antonín Lejsek Oct 28 '17 at 03:53
  • @ChrisThompson The reason the base class declares the conversion is to force every derived class to implement it. I'm not sure how that would work if there were a static class handling the conversion. I have tried to clarify the difference between `ScreenPoint` and `AbstractPoint` in my edit. – Miroslav Policki Oct 28 '17 at 05:03
  • @MiroslavPolicki If `AbstractPoint` has only one method which is the `ToScreenPoint` method, You are better off creating an interface rather than an `abstract` class. Alternately, it makes more sense to create a generic `ScreenPoint` class that has a `toScreenPoint` method that takes generic input.... – Chetan Kinger Oct 29 '17 at 14:47
  • @CKing Yes, but that interface would still be in the same kind of situation: declaring a method returning an implementer. Would that really be acceptable whereas a base class declaring a method returning an inheritor wouldn't? Besides, geometric points are just an example; you could have a large amount of common functionality in the base class. Also, could you elaborate on the generic suggestion? – Miroslav Policki Oct 29 '17 at 16:45

3 Answers3

2

I feel that the Single responsibility principle in SOLID has been overlooked. The AbstractPoint and its concrete implementation ScreenPoint, are essentially storing data, e.g. X, Y, unique to the class group. As well the base class AbstractPoint is trying to enforce what seems like the Factory Method pattern inline (minus an interface being returned). While I think it to be appropriate and necessary to have logical operations on the data in the AbstractPoint and/or ScreenPoint classes; I feel like a separate ScreenPointFactory class implementing Factory Method pattern to instantiate the ScreenPoint is much needed here.

If you are needing to create thousands of ScreenPoint classes, having the extra virtual method calls such as ToScreenPoint() and extra object size could have negative performance issues. Considering to use the Flyweight pattern in this scenario could help load times. For Flyweight pattern to be successful, you will need the Factory Method pattern to be implemented as the Flyweight pattern is used within the factory. Since there will be factories, you will need an IPoint interface and simply derive AbstractPoint off of the interface IPoint. IPoint will simply hold X, Y for now. The remaining point types gain the IPoint interface through inheritance of AbstractPoint. Since there will be group of related point types, e.g. ArbitraryPoint, ScreenPoint, they can all be instantiated via an Abstract Factory.

Programming to interfaces, not implementations principle will be important here. To pull off the functionality of ToScreenPoint() in what I am describing, I would remove ToScreenPoint() from the Point classes completely and instead, I would create an ArbitraryTransformation object configured upon startup. I would use Dependency Injection to place the ArbitraryTransformation object into the appropriate factory during IOC configuration. Then when the Abstract Factory methods are called to create any new variations of your screen points… they are created with the arbitrary transformation already calculated, as each independent factory method within the AbstractFactory will use the appropriately configured ArbitraryTransformation to perform the calculation.

Doing all this will put less stress on your design, and will keep your objects a bit lighter and loosely coupled. I feel you are dealing with complexities here to where you can figure out what I just said above simply with the GoF pattern language. However, if you or anyone would rather have the coded samples, I can come back in and provide a sample solution. Just seems like a whole lot of code to link, if you do not exactly need or want what I am suggesting.

I went ahead and developed a sample solution on my GitHub called Point Example. Let me know what you think.

Also, for every design pattern added here, it will introduce extra layers of complexity to your app, so while that can be a problem in itself, I think it will lend itself to be useful for what you need.


Update

In the point example I decided to do a very simple implementation of Object Pool pattern instead of the mentioned Flyweight pattern above. I showed placement of a performance improvement pattern in doing so in case a solid example was needed. Although there are better ways to give the implementation, the Object Pool pattern as used in the ScreenPointFactory is a mere functional place holder.

I have updated the code in the Point Example to reflect naming things Flyweight pattern to Object Pool pattern. Sorry for any confusion. Also, here are answers to Miroslav Policki's questions in the comments section below:


Question 1:

1. It seems your motivation for moving onto the Factory Method pattern is the ability to use the Flyweight pattern. However, as I understand it, the Flyweight pattern is a performance optimization, and as such shouldn't be applied prematurely. So, in situations where the Flyweight pattern is unnecessary, would you still proceed with the rest of your design, and why? – Miroslav Policki

Answer: I made a correction referencing the Object Pool pattern instead of Flyweight pattern to the code. Regardless of my naming mishap, both Object Pool pattern and Flyweight pattern are performance optimizations. However, my motivation for creating factories is out of habbit, as it is common to do so to encapsulate creational logic. Yes my implementation in the example code of Object Pool pattern was never asked for, and it is a perfomance optimization that is applied prematurely in this situation. However, I felt it to be more of an educational bonus, if somebody was wondering where to place performance enhancements in this paradigm, without spending too much time. I gave ArbitraryPointFactory no performance enhancements to show the flexibility of the factories.

Also, yes, I would absolutely use the Factories to create any variations of points, as from the given example there were mulitple concrete types. Keeping the factories around will keep instantiations of point ojbects in a single location in code, and factories will yield a single place in code to modify creation logic. Factories are useful for simple and complex objects, and allow us to program to interfaces, and not concrete implementations. However, like most design patterns, I would not suggest for anyone to have to indroduce factories into their code, if there is only one concrete point object or one place in thier code that needs to call the new operator for an object. However if you have two instances in your code where the new operator is being used to create concrete points, etc then it may be nice to have a factory so you can make a change in one place in a factory, instead of having to search through code and run into the possibilty of not updating all places appropriately if there are changes to the way the object needs to be created.

Question 2:

2. You apply the transformation on a point during its creation in the corresponding factory method. However, this is different from the semantics in my example, because each ArbitraryPoint has internal state which can change at any time, and thus yield different ScreenPoints. How would you incorporate this into your design? – Miroslav Policki

Answer: Part of me wants to say the name ScreenPoint has made me performance antsy, and I am striving for a relevant implementation. The other part of me wants to say I would have designed it this way regardless. However, I know you are suggesting ScreenPoint being pixel related, etc... is not the case, it is just another arbitrary example. I will give you my valid example first though. I was contemplating the creation of thousands apon thousands of ScreenPoint renders in a video game, possibly where we may also be storing the color of each pixel per screen point, and we are just creating a full screen render of throw away ScreenPoint objects. So lets say there is a double buffer of screen points going, where we are drawing two screen renders at a time. We would just be calculating the render of game object coordinates to real world coordinates and then mapping them to a 2D screen render 30 to 60+ times a second. In this scenario, I wouldn't care about modifying existing renders X,Y data, rather than just creating a new set of ScreenPoint objects for each additional render. Now that can seem wasteful, and Flyweight pattern could help to overcome having to create so many new objects at runtime as there would be only one ScreenPoint object, and the implemenation of each X,Y would be encapsulated to an internal Array or Dictionary. However, again I was also considering object size and how many method and virtual method pointers each object had to maintain along with the data, when creating or dealing with thousands of ScreenPoint objects very quickly when thinking of my design. The design allows ScreenPoint to be purely a data object, nothing more.

Back to your semantics though, I achieved the calculation through object composition via the ArbitraryTransformation object and related interfaces. There is nothing preventing us the ability to compose the AbitraryTransformation object with your ArbitraryPoint object or a NoOpTransformation with your ScreenPoint to be utilized in object properties or mutator methods to perform the calculation from within the ArbitraryPoint or ScreenPoint objects respectively. This is where Has-A is better than Is-A comes into play with object composition. The factory is a good place to compose your objects together with other objects, e.g. Has-A. We may later want to use a MochTransformation object in unit tests with a MochScreenPointFactory to be able to actually test a ScreenPoint object's functionality or math. So sticking with this paradigm allows for this to naturally happen in configuration (IOC Container buildup), keeping what may be a complex algorithm or impossible scenario to test against in it's own replaceable tranformation object. So again, Nothing is preventing you from keeping the ToScreenPoint() method on your ScreenPoint object if you like having the interface there instead of handing off responsibility to the factory. My re-organization of where the calculation was happening was geared towards keeping the ScreenPoint simple and small. It's single responsibility was in just keeping up with data.

I also am seeing that in your example you are using the new operator for ScreenPoint in two places, or each ToScreenPoint method. With the appropriate injected factory into each point, the factory method could be called from the ToScreenPoint() method of each type of point. There is nothing preventing this either, and there is some other point the code will be instantiating the first ScreenPoint or AbstractPoint which could be delegated to a factory instead of setup in each location a ScreenPoint needs to be created. Also, I do not like how the AbstractPoint class is returning a single concrete instance of ScreenPoint rather than an interface. You have simply embedded the notion of an Abstract Factory pattern for a family of objects, yet every family returns a concrete instance of one type, rather than an interface. Because of this your calling code will need to be context aware regardless. This is why I broke out in my example the notion that ScreenPoint and ArbitraryPoint derived from the same interface and yet ArbitraryPoint had it's own extra / custom data. If I need to perform the conversion from arbitrary point to screen point, I had to be context aware anyways enough to use the correct Factory method to create a new point with the correct calculation, which in turn utilizes the ArbitraryTransformation calculation for instance at the moment of object creation.

I think where my example may differ from yours in intent, is that you wish for me in the IOCConfig.cs to create ScreenPointFactory as follows new ScreenPointFactory(new ScreenTransformation()) to where instead of being a No-Op type operation it does the work of conversion, and that ArbitraryPointFactory had the No-Op. I would then be able to pass in the X,Y or perhaps the IPoint interface to the ScreenPointFactory or the correct PointFactory method to then do some conversion from ArbitraryPoint perhaps to what could be a ScreenPoint. So in this instance, I would be modifying the data of an ArbitraryPoint that may be a point on a video game model, and then use my ScreenPointFactory to create a ScreenPoint from an ArbitraryPoint which does all the coordinate conversions. Then instead of modifying the ScreenPoint calcs directly, I would still be modifying the video game model directly or the original ArbitraryPoint, to then throw away my generated ScreenPoint and generate a new ScreenPoint by calling the ScreenPointFactory to update my ScreenPoint location from the values of the original ArbitraryPoint. Dealing with these complexities will definitely warrent the use of the techniques I am presenting here with Factories. Also simplifying your object models to be used in a manner that makes sense to the application that is using it.

Anyways tag me back if any of what I have said is questionable or if you would like me to change the sample code around in any way to match my last paragraph. The only other way I might stick to something similar to what you have in your example is to try and implement the Adapter pattern. Then I have the ability to go from one interface to another for instance, so from ScreenPoint to ArbitraryPoint and vice versa without forcing them to derive from the same interface or base class. Yet providing some calculation and conversion back and forth. My implementing code would always look to the Adapter class to pull back the data I wish to operate on and then convert back and forth. Either way I guess the main design principles I have tried to express are (Found in the book Head First Design Patterns):

  • Program to an interface, not an implemenation.
  • Favor composition over inheritance. (Has-A is better than Is-A)
  • Dependency Inversion Principle (Depend upon abstractions. Do not depend upon concrete classes.)
    • No variable should hold a reference to a concrete class. (If you use new, you'll be holding a reference to a concrete class. Use a factory to get around that!)
    • No class should derive from a concrete class. (If you derive from a concrete class, you're depending on a concrete class. Derive from an abstraction, like an interface or an abstract class.)
    • No method should override an implemented method of any of its base classes. (If you override an implemented method, then your base class wasn't really an abstraction to start with.)

      Funny thing is the book goes forward to mention for Dependency Inversion Principle, that if you follow all three of those design guidelines with no exceptions that you would never be able to write a single program. So I will say to you that try to aim towards these principles, but only where they make the best sense. For any of the design patterns or principles there are both pros and cons. However, at least with the design patterns they are more like templates to do what you want from already solved problems. So plug them in and adapt them where they make sense to your design. Others will be familiar with your design and can jump right in with your naming strategy and add to your code.
  • The answer including the code is quite detailed; thank you for the effort. I do have some questions. 1. It seems your motivation for moving onto the Factory Method pattern is the ability to use the Flyweight pattern. However, as I understand it, the Flyweight pattern is a performance optimization, and as such shouldn't be applied prematurely. So, in situations where the Flyweight pattern is unnecessary, would you still proceed with the rest of your design, and why? – Miroslav Policki Nov 14 '17 at 00:11
  • 2. You apply the transformation on a point during its creation in the corresponding factory method. However, this is different from the semantics in my example, because each `ArbitraryPoint` has internal state which can change at any time, and thus yield different `ScreenPoint`s. How would you incorporate this into your design? – Miroslav Policki Nov 14 '17 at 00:11
  • Miroslav, my complete apologies. I have used *Object Pool pattern*, and not *Flyweight pattern* in the *Point Example* code. I will update the code and answer above with an appropriate response to your two questions. May take me a bit. However, yes I got carried away making design choices thinking of many objects due to the example being a screen point, and no it was not a full implementation of *Object Pool pattern*, more of a placeholder. – Ryan Mauldin Nov 14 '17 at 03:31
  • It would be great if you could update your code on GitHub as well, just to make sure I (or any other reader) have a correct understanding of your latest update to the answer. – Miroslav Policki Nov 17 '17 at 19:28
  • The source code is up to date with the *Object Pool pattern* rename. Was this okay as is, or were you wanting me to do add the **ScreenTransformation** class and changes I mentioned in the Update section? I was not positive if this was what you were looking for? – Ryan Mauldin Nov 18 '17 at 00:47
  • Yes, I meant the ScreenTransformation class and changes you mentioned in the Update section, if it's not too much trouble. – Miroslav Policki Nov 18 '17 at 19:35
  • Gotcha, I will try to address this at some point this week. Will notify you again when it is complete by editing this comment. – Ryan Mauldin Nov 22 '17 at 15:21
1

This kind of design is usually a code smell. Base classes should not know about their derived classes because it creates a circular dependency. And circular dependencies usually lead to complicated designs where it's difficult to reason about what classes should be doing. In Java, base classes knowing about their derived classes can even result in deadlock in some rare cases (I don't know about C#).

However, you can break general rules in special cases, when you know exactly what you are doing, specially if what you are trying to achieve is simple enough.

And your case here seems to be simple enough. Having AbstractPoint and ScreenPoint as different classes is correct. But in fact they "work together": All AbstractPoints should be able to convert to ScreenPoint (that's maybe the most important functionality in AbstractPoint's contract?). Since one cannot exist without the other, there is nothing wrong about AbstractPoint knowing about ScreenPoint.

Update

In a different design: Create an interface called CanonicalPoint. AbstractPoint has a method called ToCanonicalPoint, which returns CanonicalPoint. All derived classes of AbstractPoint have to implement this and return CanonicalPoint. ScreenPoint is a derived class of AbstractPoint which implements the CanonicalPoint interface. You could even have more than one derived class that implements CanonicalPoint. Note: If AbstractPoint and CanonicalPoint have methods in common, both can implement another interface called, say, Pointable, that declares all of these methods.

Marcelo Glasberg
  • 29,013
  • 23
  • 109
  • 133
  • That's a good observation about the conversion being the key thing in the contract. However, I'm getting the feeling that you and some of the commenters think my question is about the specific example in the code, when it's actually about the scenario the code exemplifies. While the example is simple in order to illustrate, in general the domain could be arbitrarily complex, and I'm wondering if there's a way not to break OO best practices while still satisfying the constraint that a bunch of things of the same kind have to be convertible to a canonical thing of that kind. – Miroslav Policki Oct 30 '17 at 05:29
  • Ok, I will post an alternate solution. See above. – Marcelo Glasberg Oct 30 '17 at 20:04
  • Note, above you can also play with the names. Create an interface called `ScreenPoint`. `AbstractPoint` has a method called `ToScreenPoint`, which returns `ScreenPoint`. All derived classes of `AbstractPoint` have to implement this and return `CanonicalPoint`. `ScreenPointImpl` is a derived class of `AbstractPoint` which implements the `ScreenPoint` interface. You could even have more than one derived class that implements `ScreenPoint`. – Marcelo Glasberg Oct 30 '17 at 20:31
  • Your suggestion for the base class method to return an instance of an interface that the derived class implements would certainly work, with the interface now representing the canonical representation instead of the derived class. However, this would seem to violate DRY, since you'd have to declare the same elements in both the base class and the interface. That doesn't quite feel right. – Miroslav Policki Nov 14 '17 at 00:07
  • @MiroslavPolicki Hmm, that's easy to solve. The base class and the interface could both implement another interface (say, Pointable interface) that has the methods that CanonicalPoint and AbstractPoint have in common, if any. – Marcelo Glasberg Nov 14 '17 at 17:09
  • 1
    After reviewing all the answers and comments, I find that your interface solution is the only one that satisfies the requirements of the problem. However, even then, the solution ends up being two interfaces; one of them a marker interface which doesn't really add any benefit except for the avoidance of an explicit reference to a derived class by a base class, at the expense of a more complex design. In light of all this, I'm inclined to conclude that this is one of those cases where it's justified to go against conventional wisdom, as you originally suggested. – Miroslav Policki Dec 07 '17 at 01:06
1

You have complicated things without reason. If a class cannot implement the specification of the base class you are violating Liskovs Substitution principle.

An alternative is an interface that declares that an arbitrary object can be represented as coordinates:

public interface IScreenPointProvider
{
    ScreenPoint ToPoint();
}

Then it doesn't matter what the object is or what its internal handling of coordinates is.

Also do not forget that inheritance is about is-a relationship. If any of the sub classes do not support what the base class provide, it's not really a is-a relationship. That typically indicates that the base class really is a utility class or that the defined hierarchy isn't well designed.

jgauffin
  • 99,844
  • 45
  • 235
  • 372
  • But in this case the classes can indeed implement the specification of the base class. They can all implement `ToScreenPoint()`. Even `ScreenPoint` can implement `ToScreenPoint()` by simply returning itself. This is like `Object.ToString` returning `String`, which is derived from `Object`. – Marcelo Glasberg Oct 30 '17 at 20:26
  • He stated *"an abstract point has no coordinate system information, so the values of X and Y are meaningless"*. No inheritance hierarchy should be built on a false grounds. My alternative do not use inheritance for that reason. – jgauffin Oct 30 '17 at 20:31
  • Yes, but I believe he wanted to write that "an abstract point may have some arbitrary coordinate system information, which is defined by each derived class". Clearly ScreenPoint has meaning (X,Y in the screen). He wrote (in a comment) that: "While the example is simple in order to illustrate, ... [what I really want to know is] if there's a way not to break OO best practices while still satisfying the constraint that a bunch of things of the same kind have to be convertible to a canonical thing of that kind." – Marcelo Glasberg Oct 30 '17 at 20:39
  • Abstract base classes should be used when there is a least common denominator. An empty base class do not have that, nor do a base class with properties that doesn't mean anything to the base class itself. Thus it's poor OOP design to introduce a base class then, an interface is a much better fit since it specifically add a certain functionality without dictating what the class do otherwise. – jgauffin Oct 31 '17 at 05:43
  • My point is that if the base class introduce common behavior (methods that change the object state) of point types, then yes, a base class is a good fit. However, if the base class is empty or just contain properties it's not really a valid base class in OOP sense. – jgauffin Oct 31 '17 at 05:44
  • But that's not his question. His example is simple in order to illustrate. It doesn't mean that he's asking about a base class that's empty. – Marcelo Glasberg Oct 31 '17 at 13:53
  • As MarcG has said, my question is not about the concrete code. In general, the base class would hold properties that are common to and methods that can be used on all objects of the type. I like the IScreenPointProvider solution, however it is limited: if I have a bunch of IScreenPointProvider references, I can get ScreenPoints out of them. But if I want to have a bunch of points, apply a common operation on them (for example rotate them around the origin, wherever that may be), and convert them to ScreenPoints, then I need to have AbstractPoint references, not IScreenPointProvider references. – Miroslav Policki Nov 14 '17 at 00:09