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?

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.

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.

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.