3

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?

Improve this question
10
  • I'd say, on the contrary, keep abstracting code until you can't anymore. You should read 'clean code' by robert martin, it gives plenty of reasons as to why. Commented Jul 25, 2018 at 10:48
  • 8
    @JᴀʏMᴇᴇ: We shouldn't just abstract for abstraction's sake. You can't sustainably assume that every project ever created will change in every conceivable way. There needs to be a justifiable argument in favor of abstraction, other than "just do it" (e.g. expected future changes, or the stakeholder's explicit desire to prioritize faster future development over faster current development). I'm not going to build a bigger garage today on the off chance that I will have a limo in the future. The cost of doing so does not outweight the odds of it happening. Commented Jul 25, 2018 at 10:51
  • 5
    @JᴀʏMᴇᴇ Abstraction is a tool, not a methodology. Use it to obtain a result, not to do it for the sake of abstraction. That's a bit like purposefully adding newlines in code for "readability" sake. If it doesn't make sense to do so, it is self-defeating. Commented Jul 25, 2018 at 10:51
  • 1
    @Ewan - I actually agree with you though!? I gave you a vote, I think a clear separation of concerns as you suggest is absolutely the right idea. Commented Jul 25, 2018 at 11:04
  • 1
    @JayMee - Its not specifically disagreeing with OOP, its the interpretation of it that is somewhat confusing specifically with the question I posted. Abstract to a point of singular responsibility or to the point of singular functionality. Do you brush your teeth on your own or get people to open the tap, give you the toothbrush and then you yourself brush your teeth. Won't this specific example be abstracting too much? Commented Jul 25, 2018 at 11:16

4 Answers 4

Reset to default
8

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.

Improve this answer
30
  • 6
    Disagree with this answer. Abstracting prematurely leads to higher cognitive load in comprehending code and most often lead to abstractions that turn out to be unnecessary or - more importantly - wrong and difficult to reverse. I believe that abstracting hard dependencies at boundaries of systems is a sensible starting point and all other abstraction should be driven by need. Of course different people have a different idea of what "abstraction" actually means. Commented Jul 25, 2018 at 10:51
  • 1
    I agree with @Ewan, make it a question so we can vote. Very good points are made in this post Commented Jul 25, 2018 at 11:01
  • 1
    @Flater "If it's correct and done or quick to do" If I had the super garage it would be worth more money, even if I don't have a helicopter Commented Jul 25, 2018 at 11:05
  • 2
    @JᴀʏMᴇᴇ: As per your own statement: I'd say, on the contrary, keep abstracting code until you can't anymore. So why are you drawing the line at N abstraction levels, and why not at N+1? Or N+2? ... This is the issue: every next level of abstraction needs to justify its existence. It's not a default "yes". Commented Jul 25, 2018 at 11:07
  • 3
    @JᴀʏMᴇᴇ I've seen way too many very bad examples of your logic taken to its logical end point. Which led to one program having a class to add 2 integers, with a method add(int), which was used in lue of doing like int i=a+b; THAT's the end result of your idea Commented Jul 25, 2018 at 11:15
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.

Improve this answer
3
  • I keep looking for a short 'bumper sticker slogan' way to describe SOLID to non-engineers. "You can't always know what's coming in the future." feels as close as anything. Commented Jan 19, 2021 at 16:12
  • @MarcelWilson I think "solid" is a bad adjective to describe it, I would've used "nimble", i.e. agility and flexibility. "Float like a butterfly" and all that. Commented Jan 19, 2021 at 16:15
  • I just meant I was trying to describe SOLID principles to non-engineers without having to spend 45 minutes going over each letter in the acronym. I agree that nimble and agile are perfectly fine but they already 'know' those words. I kept looking for a succinct way to describe the whole concept. Something execs can chew on and feel they understand in a way that felt 'good enough' for me. Your line about the future felt like it really hit the mark. I plan on using it soon to re-emphasise the concept. Commented Jan 20, 2021 at 15:17
1

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.

Improve this answer
0

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); } }
Improve this answer
7
  • @Flater here is a different example of the code for a clearer example of what is happening Commented Jul 25, 2018 at 13:42
  • wait, so you already have two classes and the argument is whether to add a third? Commented Jul 25, 2018 at 14:05
  • whats wrong with RepositoryFactory.GetRepository<Model>().GetAll() ? Commented Jul 25, 2018 at 14:05
  • @Ewan See this was the original question that caused this issue; whether or not to move the GetAll method out of the RepositoryFactory or not. SRP would say "yeah man go ahead" because the Factories job is just to create repositories, not execute methods on them. Commented Jul 26, 2018 at 7:23
  • sure, but it doesnt say 'make a helper class that does nothing' Commented Jul 26, 2018 at 7:25

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.