Refactoring into sub classes

Question

public class ScheduleRatesController
    {
        protected CoreDataManager dataManager;

        public ScheduleRatesController()
        {
            dataManager = new CoreDataManager();
        }

        // testing
        public ScheduleRatesController(CoreDataManager manager)
        {
            dataManager = manager;
        }

        public virtual void GetTranQuotesToFillRatesAndPayments(ref List<int> ids)
        {
            ids.AddRange(new List<int>());
        }
    }

So to give you guys some background, we're splitting one DB query into a bunch of different ones, and we want subclasses to basically each take on a DB call for their GetTranQuotesToFillRatesAndPayments() method that represents it's specific query.

What you see above is the base class I have. I made those two methods virtual as I plan on having subclasses override them to perform their own stuff. So one could be like:

public override void GetTranQuotesToFillRatesAndPayments(ref List<int> ids)
        {
            ids.AddRange(dataManager.GetLoanTranQuotes());
        }

and etc. My question is, is this the best/cleanest way to perform a pattern like this?

The code that calls this is going to contain a huge list of filtered id's, that it's going to need to fill by calling each classes call to GetTranQuotesToFillRatesAndPayments(). Let me know if this doesn't make sense. I'm kind of getting turned off by the fact that I'm going to need to call the same method like 6 times, each on a different class. I think that might be messy in itself even though the goal of it was to make it clean. I don't want to have something like this on the calling side:

List<int> ids = new List<int>();
ScheduleRatesController controller = new LoanController();
controller.GetTranQuotesToFillRatesAndPayments(ref ids);

controller = new TradeController();
controller.GetTranQuotesToFillRatesAndPayments(ref ids);

etc.

Let me know if you need any more background or info.

Thanks.

How do you want to use it? You can have a Facade class wrapping other objects and exposing a similar method. — Dmitry Shkuropatsky, Jan 16 '12 at 15:42
It's pretty hard for me to figure out what exactly you are trying to achieve. A minor change that I should make is to give the "GetTranQuotesToFillRatesAndPayments" a return type of List instead of passing it as a reference. And by the way a List is already a reference type isn't it? — Youp Bernoulli, Jan 16 '12 at 15:44
Okay so no ref, that was a smaller issue. But @DmitryShkuropatsky - I basically want to use this so that each subclass calls a different query in the DB, filters it's own list of id's that come back, and then adds them to my list in the calling class so I have one big list at the end that I can work with. JoepGreuter - I'm basically trying to achieve cleanliness. — slandau, Jan 16 '12 at 16:06

score 2 · Accepted Answer · edited May 23 '17 at 10:34

2

Several design remarks:

Using the ref keyword usually indicates design problems and should be avoided. There is no need to pass a reference value using the ref keyword (any List<T> is always passed by reference). Your program would work equally without it.
A better idea than passing your list to the method would be to return your data from the method, and allow callers to decide what to do with it. Maybe you will only want to find a single value at some other place in your program, and creating a new list is an overkill. Also, you should try to add as little functionality as possible to each class (Single Responsibility Principle), and your class is right now responsible for fetching the data and deciding how it should be stored.
Naming: your method name is really complex. Also, the name "controller" doesn't usually represent an object responsible for fetching data.
On the other hand, you have a CoreDataManager class (btw, Manager is a bad suffix for any class), which appears to contain a bunch of methods which return various data. What is the need for ScheduleRatesController then? Does it only copy this to a list?
Business logic should be separated from your Data access layer. You should consider using Repository pattern, or similar (check this answer, for example), to ensure that your data class only fetches the data from the DB.

If you have several classes which need to fulfill a certain contract, start by creating the interface which they need to implement. Don't think about reusing code at this time. Your code, for example, forces all subclasses to use the CoreDataManager, while one day it may turn out that a certain "controller" might need to be composed of different objects.

edited May 23 '17 at 10:34

Community

1
1

answered Jan 16 '12 at 16:25

vgru

49,838
16
120
201

Well it's the filtering we really need the separation for. We can definitely use CoreDataManager as the only class that gets the data, but each proc we use to get a subset of data needs to be filtered differently. Does that make sense? – slandau Jan 16 '12 at 16:57
@slandau: well, it is not exactly clear how they should be filtered. It would help if you posted a couple of different ways to filter them, to explain better what are the differences between those classes. If you need to apply several filters to the same data (like a chain of filters), then you can use the approach [Charles described below](http://stackoverflow.com/a/8883265/69809): simply create a list of delegates and apply them sequentially. On the other hand, if you are using an ORM which supports LINQ (EF, NHibernate), then it's a completely different thing. – vgru Jan 16 '12 at 17:04
ah I see. Okay, so the filtering actually isn't that clear cut. It's basically using a lot of our other business classes to load up transactions given those Id's, and remove the ones that shouldn't be worked with. We are doing this in the DB right now, but we want to move this into the code. – slandau Jan 16 '12 at 17:06
@slandau: this part is not entirely clear to me (that's why I mentioned LINQ): filtering is usually best done no the DB side, because otherwise you need to load the entire collection into memory, which defeats the purpose of having a database. With proper SQL indexes, there is little possibility that you would make it more efficient on the client side. LINQ providers in EF and NH translate coded queries into SQL-side queries to delegate this to the server. – vgru Jan 16 '12 at 17:10
Yeah, I mean I kind of agree. Our DB is a huge thing though not really index'd properly but that's a longer project – slandau Jan 16 '12 at 17:11

score 0 · Answer 2 · answered Jan 16 '12 at 16:45

Use a List<Func<List<int>,List<int>>>. Which is basically a list of functions with the following type signature:

List<int> MyFunc(List<int> foo);

You can then pass the list of functions to a method that works like the following:

public List<int> GetAllIds(List<Func<List<int>,List<int>>> functionList) {
    var listOfIds = new List<int>();
    foreach(var f in functionList) {
        listOfIds = f(listOfIds);
    }
    return listOfIds;
}

You can use lambdas to compose functionList like so:

functionList.Add(list => {
   list.AddRange(dataManager.GetLoanTranQuotes());
   return list;
});

Now you do not have to depend on any specific inheritance hierarchy. You can use function composition to produce the whole list.

Refactoring into sub classes

2 Answers2