Single Responsibility Principle, Inheritance

Question

I have the following interface:

class IHittable
{
    virtual Intersections intersects(const Ray & ray) = 0
}

which will be implemented by various geometric objects(Spheres, Triangles etc). Now the class World will look like this:

class World
{
  public:
    Intersection intesects(const Ray & ray)
    {
      auto intersections = Intersections();
      for(auto object : objects)
      {
        auto xs = object.intersection();
        if(xs.size() > 0)
        {
          intersections.append(xs);
        }
      }
      return intersections;
   }
  private:
     std::vector<std::shared_ptr<IHittable>> objects;
}

My question is, should the class World also implement the IHittable interface? because its member looks the same and does exactly the same thing? How bad is it if I don't do that?

A second more generall question if this is allowed: When should I make a function free standing and when not, in my case the function intersects inside the World class could be easily made free standing just with one extra argument. I don't know how to think about such cases.

Am I going against the "Single Responsibility Principle" if Intersects is a member of World?

Answer 1

My question is, should the class World also implement the IHittable interface? because its member looks the same and does exactly the same thing?

It would be a reasonable choice to make World implement IHittable because it gives you the option to add an instance of World to another World (or generalize the idea of World and rename it Container). This would be an example of the Composite pattern (https://en.wikipedia.org/wiki/Composite_pattern).

How bad is it if I don't do that?

If you never intend to put a world in a world, there is no harm at all to not implement IHittable. After all your design is not about getting it right, but what serves your use case the best. If your code will never treat a World object as an IHittable, then World shouldn't implement that interface.

A second more generall question(subquestion) if this is allowed: When should I make a function free standing and when not, in my case the function intersects inside the World class could be easily made free standing just with one extra argument. I don't know how to think about such cases.

This ends up being a question about your object boundaries and abstraction. There is no easy answer to these types of question, since it takes some experience to get to the right abstractions specific use case. But I give you some things to consider: If intersects is an instance method, you can make World implement IHittable If you want to make intersects a static method that takes both a Ray and a World then you need to be able to access the list of IHittable objects the world contains When you make objects public to allow access from an static method, your World object ends up just being a data structure. That works but you end up losing all the aspects of object oriented programming (whether that is a good thing or a bad thing).

Answer 2

If it is just a coincidence that the method has the same name and signature and its use case is different from the IHittable interface method, you should rename it to avoid confusion.

My question is, should the class World also implement the IHittable interface?

This is not a question, it already does. You probably want to know whether you should declare class World as being an IHittable. You should only if you would ever want a World object to be treated as an IHittable. World does not sound like a body like the examples you named so from the code I am not so sure it would be appropriate. Testing if a body is in your world could be considered a hit test but it is not in the sense of a collision so you may want to model and name that separately.

Answer 3

My question is, should the class World also implement the IHittable interface?

Yes, this is exactly what interfaces are for. Doing so lets you pass a World to any function that takes in IHittable. In fact, your World class is an example of the composite pattern - it is a group of IHittable objects that is treated as a single unit, i.e. from the outside it looks like a single IHittable.

When should I make a function free standing and when not, in my case the function intersects inside the World class could be easily made free standing just with one extra argument.

At least in C++, free functions are generally preferred if they can be implemented using only the public interface of an object. In your case, you would need to make the list of objects in World publicly accessible if you want to implement intersects as a free function. Additionally, you lose runtime polymorphism with this approach, which is essential for the composite pattern. Of course, it is possible to come up with a different design that does not involve runtime polymorphism at all, but if you're going for an object-oriented design, you're more likely to use member functions rather than free functions.

Am I going against the "Single Responsibility Principle" if Intersects is a member of World?

Not at all. The "single responsibility" of World could be to model a collection of IHittable objects but that could entail several bits of functionality, including intersects.

Answer 4

Already some very good answers here, in particular Helena's. I'd nevertheless would like to add a couple of ideas missing in the other answers.

Member function or not?

The C++ Core guidelines, recommend:

Make a function a member only if it needs direct access to the representation of a class

In your specific case, IHittable are shapes, and you need to determine the intersection with some Ray. You'd probably determine such an intersection using all the available geometric information of the shape, whether public or not.

Moreover, you'd do it differently for circles, triangles and rectangles, and you need to know the real nature of the shape at run-time to determine the right function. This is not easy with free-standing functions, since you can benefit of overloads (compile-time) but would need to add some visitor logic to do this dynamically depending on the shape of the IHittable (run-time). Virtual functions is doing it much better out of the box: therefore keep it a member function.

Hello, IHittable World?

The beauty of OOP is that sometimes we discover on the road that some abstract ideas are more related that envisaged. And when it makes sense we refactor. Is it the case for IHittable and World ? Three criteria can help to decide:

1. Could you imagine using World, wherever you use a IHittable (Liskov's substitution principe)?
2. Would World::intersect() signature and meaning change like IHittable::intersect() and for the same reason?
3. Would it have any advantage ?

If the answer is yes on all the three, you may consider inheritance in C++ (or interface extension if you'd work in another language where IHittable would be an interface).

Single responsibility principle

You are not going against the principle of single responsibility having intersect() a member of World. SRP is about a single reason to change, as R.C.Martin who invented it explains. And SRP is not incompatible with inheritance, as explained here.

Answer 5

My question is, should the class World also implement the IHittable interface? because its member looks the same and does exactly the same thing? How bad is it if I don't do that?

I think it should, it already fulfills the contract so is there any downside to allowing it to be used as a IHittable? It also doesn't hurt to leave it out for now and then just add the interface as you need it.

When should I make a function free standing and when not

In this case I think you would not use a free function because the parameter you would add is an implementation detail of this type of IHittable. Other types like Sphere or Box have no need for objects since they will determine intersections based on calculations.

Am I going against the "Single Responsibility Principle" if Intersects is a member of World?

No but one thing I would add is while your design does not break the rule yet, the name World might cause you to add other different kinds of responsibilities later.

For example, you may add a weather system to your world and now the World no longer has a single responsibility. Instead I would consider renaming to something that reflects the object as a collection of IHittable: CompositeHittable, HittableList, Hittables. Now the concept is more clear it will be easier to figure out where responsibilities should lie, wouldn't it feel weird adding a weather system here?