Is Abstracting your code too much a bad use of SOLID Principles?

Question

After an unresolved argument with a friend I decided to ask the stack overflow community. Is there something like abstracting your code too much? Which of the following is the better option. We come from different coding languages so I am explaining this the same way I would to him. Assume everything below as summaries to what your respective languages would do.

IMPORTANT CONDITIONS THAT WILL ALWAYS BE EXIST:

• There is only 1 table and 1 db
• There will only be a GET_ALL. No partial or specific collections.
• FACTORY CLASS and HELPER CLASS will always be called together NO MATTER WHAT.

FACTORY CLASS {

  //This method will check cache and create a repository instance of the table if one does not exist
  createNewRepository();

  //This will collect the records from the instantiated repository  
  getAllFromRepository();

  //This will return the data that was collected   
  returnData; 
} 

or

FACTORY CLASS {

  //This method will check cache and create a repository instance of the table if one does not exist   
  createNewRepository(); 
}

HELPER CLASS {

  //This will collect the records from the instantiated repository  
  getAllFromRepository();

  //This will return the data that was collected   
  returnData; 
}

We both have different views on the SOLID principles apparently.

I believe a class should have single responsibility in the sense of "This factory class' responsibility is to get me the data from the db" that will include the functionality required to achieve this.

He believes a class should have single responsibility in the sense of "This factory class' responsibility is to spin up the repository and not to call the data. A helper class will call the data"

I agree with him if it was the case that there was different variations of how the data is called but because the data will always be checked>createdIfNull>collected and this entire process is the functionality of "accessing the data to use" shouldn't it be in a single class?

Answer 1

I would side with your enemy on this one.

• The 'create a database and table logic' is clearly technically a separate responsibility from the 'get data from database' logic.

• You can imagine the creation code getting pretty complex if we consider database migrations etc. Enough lines of code to justify a separate file.

• You might want to restrict calling code to either creating or retrieving

In general, the time cost of making something more generic like this is hard to judge. One person might find it onerous and long winded another might do it as standard and think it quicker.

In my view YAGNI is never a good argument. If it's correct and done or quick to do, then its academic whether someone thinks they might not need it.

Answer 2

"This factory class' responsibility is to get me the data from the db"

That's like saying "I'm going to use this car manufacturer to drive to work". You're not. You're going to use a car that was built by this manufacturer to drive to work.

The car's responsibility is to let you drive around. The car manufacturer's responsibility is to build cars.

I understand where you're coming from. We all make similar "shortcut mistakes" at one time. But your assumption relies on the notion that a repository is only ever used for collecting all data.

While that may be the case today, it might not be the case tomorrow.

SOLID predominantly focuses on the future. Implementing SOLID doesn't make your (already working, non-SOLID) code work better. Implementing SOLID ensures that it will be easier to implement future changes.

IMPORTANT CONDITIONS THAT WILL ALWAYS BE EXIST:

When I take your assertion at heart, that nothing will ever change, then implement SOLID is irrelevant.

However, whenever statements like these are made, the speaker is usually not in a position to guarantee that things will never change. They're looking at it from today's point of view, not tomorrow's.
You can't always know what's coming in the future. Maybe you start using a different data provider, maybe you now need to merge data from two data providers, ...

---

That being said, I'm not quite sure what the purpose of the helper is.

• Why is this helper's method not part of the repository itself?
• What's the meaningful difference between calling the repository method directly, and calling the helper method which calls the repository method?

That may be a detail that you intentionally omitted from your question for the sake of clarity. In either case, my answer stands: retrieving the data and instantiating the class that retrieves data are two completely separate things.

Masons are not brick walls. Car mechanics are not cars. Wheat farmers are not wheat. Repository factories are not repositories.

Answer 3

If you have a Factory, that means you found the need to decouple the implementation of an object from its contract. Factories are good at that. In this case, the dependency is on what gets the data.

If the Factory is now responsible for getting the data too, what was the point all along? Now the dependency is on the Factory. It stops looking so much like a factory. It has a static repository, and now static methods for accessing that static repository. Any class using this factory is now coupled to the way the factory gets the data.

The goal should be to have the cached Repository be the only static element in this whole system. That way how the Repository itself acts is irrelevant to whoever asks for it.

---

You can do this by acting on the Factory's created Repository:

// Creates and/or gets a cache of the repository
Repository repository = RepositoryFactory.GetRepository();

// Get all the data from the repository
data = repository.GetAllData();

---

You can do this by Implementing the Singleton Pattern:

class Repository 
{
    public static readonly Instance = new Repository();

    private Repository() {
        // Connect to the repository here.
    }
}

And then acting on it:

Repository repository = Repository.Instance;
data = repository.GetAllData();

---

You can do this using a Factory Method:

class Repository {
    private Repository() {
       // Connect to repository here
    }

    public static Repository GetRepository() {
        if (cached) {
            return CachedRepository;
        }
        else {
            return new Repository();
        }
    }
}

And then acting on it:

Repository repository = Repository.GetRepository();
data = repository.GetAllData();

---

In all cases we can inject the dependency however we want to the class that requires it.

NeedsTheData d = new NeedsTheData(Repository.Instance);

NeedsTheData d = new NeedsTheData(Repository.GetRepository());

And in the Factory's case, the factory should be the one taking care of decoupling, so we don't need to inject the Repository class.

Answer 4

I feel the original code example leaves a lot to the imagination:

FIRST EXAMPLE:

public class Repsitory<TModel>
{
    public TModel GetAll(Func<TModel, bool> where)
    {
        //Will retrieve the data based on the where clause from the db
    }
}

public class RepositoryFactory
{
    public TModel GetAll<TModel>(Func<TModel, bool> where)
    {
        return GetRepository<TModel>().GetAll(where);
    }

    private Repsitory<TModel> GetRepository<TModel>()
    {
        //Checks if the cache contains an instance of the Repository<TModel> and returns it

        //IF no instance exists will create a new instance and cache it

        //Returns the new instance
    }
}

SECOND EXAMPLE:

public class Repsitory<TModel>
{
    public TModel GetAll(Func<TModel, bool> where)
    {
        //Will retrieve the data based on the where clause
    }
}

public class RepositoryFactory
{
    public Repsitory<TModel> GetRepository<TModel>()
    {
        //Checks if the cache contains an instance of the Repository<TModel> and returns it

        //IF no instance exists will create a new instance and cache it

        //Returns the new instance
    }
}

public class RepositoryHelper
{
    public TModel GetAll<TModel>(Func<TModel, bool> where)
    {
        return RepositoryFactory.GetRepository<TModel>().GetAll(where);
    }
}