C# Am I using proper SOLID principles?

Question

I am currently practising using the SOLID principles in C#/.NET

I have made a little example, but i am not sure if i have followed it correct. Its a simple example where i have an API controller that calls a method to get a list of users from an database and returned as DTOs.

To use the method i need to submit an IUserReader that is mainly doing the reading operation from the database and then a IMapper object is needed that will handle the mapping from database entity to DTO.

I made it that way to be available to switch out the IUserReader and IMapper so i can fit the query and mapping to match my requirements for a specific API call. Lets say an app is using the API to get users, the app will only need some details of the user, so we use a implementation that only selects the required information and maps it correct. We also have an backend to view the users, where we want all the user details from the database, here we use another implementation for that.

Here is my code example, i would like to know if it is done correct.

Data readers

public interface IDataReader<T>
{
 IEnumerable<T> Read();
}
public abstract class DataReader<T> : IDataReader<T>
{
 private protected IDataContext _context;
 public abstract IEnumerable<T> Read();
 public DataReader(IDataContext context) 
 {
 _context = context;
 }
}
public interface IUserDataReader : IDataReader<IUserEntity>
{
}
//First implementation of user reader
public class UserDataReader : DataReader<IUserEntity>, IUserDataReader
{
 public UserDataReader(IDataContext context) : base(context) { }
 public override IEnumerable<IUserEntity> Read()
 {
 return _context.Users.Where(x => x.IsActive).OrderBy(x => x.Name).ToList();
 }
}
//Secound implementation of user reader
public class UserGridDataReader : DataReader<IUserEntity>, IUserDataReader
{
 public UserGridDataReader(IDataContext context) : base(context) { }
 public override IEnumerable<IUserEntity> Read()
 {
 return _context.Users.OrderBy(x => x.Name).ToList();
 }
}

Now the mappers:

public interface IMapper<I, O>
{
 O Map(I item);
}
public interface IUserMapper : IMapper<IUserEntity, UserDTO>
{
}
public class UserMapper : IUserMapper
{
 public UserDTO Map(IUserEntity item)
 {
 return new FullUserDTO
 {
 Name = item.Name,
 Email = item.Email
 };
 }
}

The API then calls this class and method to read:

public class UserReaderService
{
 private IUserDataReader _reader;
 private IUserMapper _mapper;
 public UserReaderService(IUserDataReader reader, IUserMapper mapper)
 {
 _reader = reader;
 _mapper = mapper;
 }
 public IEnumerable<UserDTO> Read()
 {
 IEnumerable<IUserEntity> userData = _reader.Read();
 IEnumerable<UserDTO> users = userData.Select(x => _mapper.Map(x));
 return users;
 }
}

API method:

 public void GetUsers()
 {
 IDataContext context = new DataContext();
 IUserDataReader userDataReader = new UserDataReader(context);
 IUserMapper mapper = new UserMapper();
 UserReaderService ur = new UserReaderService(userDataReader, mapper);
 
 ur.Read();
 }

Is that correctly using SOLID principles and generally abstraction? Could i throw the IMapper dirrectly into the DataReader and make the mapping directly in the linq query instead of first returning the data result and then use mapping or would it break the SOLID principles?

• CobyC: Do not add the IMapper directly to the DataReader because you then give the DataReader 2 functions (breaking S), reading data and mapping it to the DTO. your DataReader is used to read the data. DTOs are created just before sending it over the wire. If your GetUsers() is within a controller you should inject IDataContext, IUserDataReader, IUserMapper using DI via the controller constructor.
• Martin C: @CobyC Thanks for the comment. I was pretty sure too that it would break the SOLID, if i moved the mapper. Normally i would use dbContext.Where(...).Select(x => MAP(x)) to grap data and ensure the query only selects the needed columns, but with this example i can't control the query unless i return it as IQueryable and then make the mapper add the Select() to it, but to me that sounds like bad pratice. Lets say i want to select all users like i do in UserGridDataReader but i only want the name column to be selected, do i create a new DataReader then? :)
• CobyC: You can create UserGridDataReader : DataReader<IUserEntity>, IUserGridDataReader where IUserGridDataReader will inherit from IUserDataReader where IUserGridDataReader has the required implementation for eg. ReadUserNamesOnly(). (the O part)
• Sᴀᴍ Onᴇᴌᴀ: @CobyC Please add an answer instead of a comment. Refer to the section When shouldn't I comment? on Comment everywhere.
• BCdotWEB: The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.

Answer 1

I suppose the simplest thing is to go through the principles:

• SRP - Cannot see any 'too big' things in your example
• OCP - The example is small enough and you are not using class inheritance that I cannot think of how this would apply
• LSP - You only have one implementation of each interface so hard to say
• ISP - Your interfaces seem to serve a single purpose that might serve the needs of several users - so no complaints there
• DIP - You have pushed the more variable 'decision' making out, keeping the API more abstract which seems reasonable.

Much of this, of course, depends on your specific aims and requirements. some thoughts:

• How likely is it that the mapper and reader will vary independently?
• Are you going to get any reuse out of all the different readers and mappers (actual not future-proofing)
• Will each reader have a mapper in practice, in which case your comment about combining the mapper and reader is worth more thought.
• Do you really need an interface for the reader, it seems like you are just wrapping what could be a function/lambda
• What is the motivation in restricting the outbound data?
• I appreciate this may just be an exercise, but it's good practice to think if you need to do this kind of restriction

It can be difficult to have a good instinct for principles like SOLID on toy problems due to the lack of realistic constraints and compromises.

• Martin C: Thank you ! Great response, it gave me some great thoughts about how to handle things. Can you explain what you mean by: What is the motivation in restricting the outbound data?
• CobyC: I agree with the reader having a function or lambda. The reader could easily have a function eg IEnumerable<T> ReadExpression(Expression<Func<T, bool>> expression) adding a lot more flexibility to the DataReader
• Jamie Stevenson: What is the motivation in restricting the outbound data? Basically: What requirement(s) are driving this? Security, minimising payload size, etc. I try to periodically reality check is something is required at all

Answer 2

IMO returning IEnumerable<T> from an API is a violation of "L", as running GetEnumerator() a second time (or foreach, or Count()) behaves differently for different kinds of IEnumerable<T>s. It may fail, run forever, or have expensive side-effects (like running a database query).

Instead commit to returning an in-memory collection, and type the API as ICollection<T>, or IList<T>, or return IQueryable<T> and make the caller decide how to structure and when to execute the query.

And given the uncertainty about the behavior of the returned IEnumerable<T>, the calling code has little choice but to immediately call .ToList() on the IEnumerable<T>, often creating a second copy of the collection.

• psaxton: Great considerations about returning IEnumerable<>. I have often struggled with whether to go with the (IMO) least derived IEnumerable<> or IList<>/ICollection<>. The boundary for List vs Collection to me is if the item order matters, but this gives me some tests to perform when I'm considering IEnumerable. +1!
• Andy: Returning IEnumerable has nothing to do with violating LSP.
• David Browne - Microsoft: Technically it's probably IEnumerable<T> that violates LSP, as you can't substitute a infinite streaming IEnumberable<T> for a List<T>. The point is that IEnumerable<T> is to abstract to expose in an API as you need to know additional information about it to write correct programs. System.IO.Stream has similar problems, but comes with CanRead, CanSeek, etc methods you can use to interrogate the stream capabilities.
• Andy: Why couldn't you? The IEnumerable<T> says nothing about the implementation detail, and without providing any further information on a method directly, should a parent method returning an infinite stream with an IEnumerable<T> return type be substituted for a child instead returning an instance of List<T>, the program will still execute just fine, you can run foreach over the collection, call the extension methods,... That is, as I have said, unless the parent method provides a distinct warning, that an infinite stream must always be returned. Then sure, you would break the LSP.
• David Browne - Microsoft: If an infinite stream is substituted for a List, calling code that runs .ToList() may start failing with an OutOfMemoryException, and code that calls .Count() will never terminate.

Answer 3

As requested, I moved my comments into an answer.

Do not add the IMapper directly to the DataReader because you then give the DataReader 2 purposes (breaking S):Reading data and mapping it to the DTO.

Your DataReader is used to read the data. DTOs are created just before sending it over the wire.

If your GetUsers() is within a controller you should inject IDataContext, IUserDataReader, IUserMapper using DI via the controller constructor.

If you use DI then you can create an interface for the UserReaderService and only inject that into your controller constructor, the DI container will resolve the constructor for your UserReaderService and your controller constructor will only take 1 constructor parameter.

the question in your comment:

Lets say i want to select all users like i do in UserGridDataReader but i only want the name column to be selected, do i create a new DataReader then?

You can create UserGridDataReader : DataReader<IUserEntity>, IUserGridDataReader where IUserGridDataReader will inherit from IUserDataReader and IUserGridDataReader has the required implementation, for eg. ReadUserNamesOnly(). (the O part).

Keep in mind you don't want to create an interface for each additional read function, that would be stepping towards not keeping things DRY.

You could add a method to the DataReader that has the signature of IEnumerable<T> ReadExpression(Expression<Func<T, bool>> expression) giving the DataReader more flexibility but still staying within the S principal.

The DataReader still just has one purpose of just reading data.

@Jamie Stevensons answer is great for determining SOLID, but like he said it is a pretty small example to really test the constraints.

The only other thing I noticed is that your GetUsers() API call returns void this should probably be IEnumerable<UserDTO> or List<UserDTO> unless the code was copied from a Test where you should have something like an Assert. or some indication that it was a test.