DISCUSSION: Dependency Inversion Principle Violation

[mtoncic]

Hi all,

Abstractions should not depend on details.
Here we're violating this by using the concrete implementation of the collection interface.
If in the concrete implementations we change List to something else that implements the same interface our contract would need to change. So we are strongly coupled.

For instance:

 public partial interface IApiBroker
 {
     ValueTask<List<Teacher>> GetAllTeachersAsync();
 }
 
 public interface ITeacherService
 {
     ValueTask<List<Teacher>> RetrieveAllTeachersAsync();
 }

Instead we should move to the abstraction and in the underlying implementations, if we need some concrete implementation, we can use it.
One reasonable choice would be IEnumerable. That's okay.

But, on the other side I believe that the data that we consume from other services/APIs should not be changeable.
Therefore we could use IReadOnlyCollection and enforce that collections are not changed.

I know that we can again change the underlying props of the collection objects but that's a different story.

What's your opinion on this?

[hassanhabib]

@mtoncic I'm also leaning towards IEnumerable.
But IEnumerable is a deferred execution data structure. So unless you call .ToList() the list can change. There's really no need for that level of execution on the API level. Because it materializes once the API call is done.
The other thing I have with IReadOnly is that I am not sure I want to prevent the engineers from modifying the list on it's way to a controller or a component. Why should we prevent them from that?
In all honesty, I can't really come up with a good scenario where someone would need to change the list. Seems like a bad pattern to me, instead people should do a .Select and copy the result in a new list rather.

Between IEnumerable and IReadOnly I feel that IEnumerable is more generic and less limiting. But then the deferred execution story makes me a bit hesitant.

@mtoncic what are your thoughts on this?
@BrianLParker @ShriHumrudha feel free to chime in on this please.

[BrianLParker]

You can materialise an IEnumerable at anytime. However if you materialize a large amount of data in a controller I think is bad design.

IEnumerable describes behavior, while List is an implementation of that behavior. When you use IEnumerable, you give the compiler a chance to defer work until later, possibly optimizing along the way. If you use ToList() you force the compiler to reify the results right away.

For example using IQueryable allows for the passing of the query to the database engine. ToList blocks this behaviour

[mtoncic]

Good points you've made.

It depends mostly on the use cases that we're going to have.

Why I brought IReadonly in the first place - when I request something from a service/API and it returns me results it makes sense to me NOT to mutate that but if there are some changes to be made these changes should produce another result.
Maybe in this case is an overkill. Therefore I agree @hassanhabib to go for IEnumerable.

I don't see really a problem with deferred execution. Once you need to do some processing you'll materialize it.
Some additional ToList() calls but abstraction is there and at some point when we need some specifics we can change that in the underlying interface implementations and not change the interface.
Also in case @BrianLParker mentioned, tomorrow if we have some big set of data and we need to do some processing before passing it to the client IEnumerable would be better choice.