First, I'm not sure if this is the right forum (Architecture), but because it is related with layers, dependencies and code readability, I couldn't find any better place...

Anyway. The question is: Can/Should I use IPredicate interface as a method parameter to get a list of entities?

It seems that it gives me a lot of flexibility just because the list can be filtered by any field of the main entity.

Here is the sample code:

How the consumer uses it...

                //Filter the package details by accountID
                PredicateExpression predicate = new PredicateExpression();
                predicate.Add(PackageSubscriptionDetailFields.AccountId == packageDetails.AccountId);
                predicate.Add(PackageSubscriptionDetailFields.Active == true);

                // Get the last package detail. 
                EntityCollection<PackageSubscriptionDetailEntity> details = packageManager.GetSubscriptionDetailList(predicate);

On the package manager side...

        public EntityCollection<PackageSubscriptionDetailEntity> GetSubscriptionDetailList(IPredicate predicate)
        {

            EntityCollection<PackageSubscriptionDetailEntity> coll
                = new EntityCollection<PackageSubscriptionDetailEntity>(new PackageSubscriptionDetailEntityFactory());

            IRelationPredicateBucket filter = new RelationPredicateBucket();

            // If predicate is null, no filter applied.
            if (predicate != null)
                filter.PredicateExpression.Add(predicate);
...

I know that the manager code must trust its caller because I don't know how to inspect what is inside the predicate parameter. But it is not a big problem for now.

So... is it ok? Critics?

Hello,

I think it mostly depends on how much abstraction you want between your UI and your lower layers....

About your code :

//Filter the package details by accountID
                PredicateExpression predicate = new PredicateExpression();
                predicate.Add(PackageSubscriptionDetailFields.AccountId == packageDetails.AccountId);
                predicate.Add(PackageSubscriptionDetailFields.Active == true);

                // Get the last package detail.
                EntityCollection<PackageSubscriptionDetailEntity> details = packageManager.GetSubscriptionDetailList(predicate);

From my point of view, it's not a big problem to pass entities between tiers for editing or displaying as long as complex business logic isn't required. However, I would avoid letting the upper layer dealing with predicates... At least not that way.

1) As you have mentioned, the caller has to be careful about the content of the PredicateExpression

PredicateExpression predicate = new PredicateExpression();
                predicate.Add(PackageSubscriptionDetailFields.AccountId == packageDetails.AccountId);
                predicate.Add(PackageSubscriptionDetailFields.Active == true);

                // Get the last package detail. Option 1
                EntityCollection<PackageSubscriptionDetailEntity> details = packageManager.GetSubscriptionDetailList(predicate)

is more error-prone than

                // Get the last package detail. - Option 2
                EntityCollection<PackageSubscriptionDetailEntity> details = packageManager.GetSubscriptionDetailByAccountId( packageDetails.AccountId, true  )

2) Option 2 is also much more readable and offers better abstraction since the caller doesn't have to care about all the DB filtering stuff. He just queries the entities using a nice, and comprehensive API.

3) If you use Option 1, you are likely to end up with a lot of duplicated code in case you might need to write the same filters from different places in your code.... Option 2 would be a reusable as it.

4) If you're going to use unit-testing, no doubt Option 2 is better.

I think the problem with your solution is that your manager class doesn't hide the database complexity as much as it should. I'm not saying you should totally abstract your database so that the consumer of your BLL would be left in his fancy object-oriented world. Nevertheless you could give him a nice and reusable API to do most of the job. Even if you're working alone on your project you'll enjoy maintaining a clean, easily readable code

Hope this helps , if you have more questions, don't hesitate

2) Option 2 is also much more readable and offers better abstraction since the caller doesn't have to care about all the DB filtering stuff. He just queries the entities using a nice, and comprehensive API.

3) If you use Option 1, you are likely to end up with a lot of duplicated code in case you might need to write the same filters from different places in your code.... Option 2 would be a reusable as it.

4) If you're going to use unit-testing, no doubt Option 2 is better.

I think the problem with your solution is that your manager class doesn't hide the database complexity as much as it should. I'm not saying you should totally abstract your database so that the consumer of your BLL would be left in his fancy object-oriented world. Nevertheless you could give him a nice and reusable API to do most of the job. Even if you're working alone on your project you'll enjoy maintaining a clean, easily readable code

Hope this helps , if you have more questions, don't hesitate

I do agree that GetSubscriptionDetailByAccountId is more explicit about what it is doing, so it must be easier to test. I also agree that it may not be the best abstraction of the database, although can hide some freak ORM code (sorry, but RelationPredicateBucket is something I'd like to hide! ).

But since it is more specialized on what it does, i don't agree that makes it more reusable. Actually, I think this is the point: I don’t want to end up with GetListById, GetListByAccount, GetListByThis, GetListByThat. The idea is to have a more reusable GetListBy[Almost]WhateverYouWant.

Right?

But since it is more specialized on what it does, i don't agree that makes it more reusable. Actually, I think this is the point: I don’t want to end up with GetListById, GetListByAccount, GetListByThis, GetListByThat. The idea is to have a more reusable GetListBy[Almost]WhateverYouWant.

If the caller needs to get the same filtered list from different places in the app (projects?), he'll have to redeclare the predicateExpression so it's less reusable... but it's more flexible.

Anyway, it's up to you to decide which option is the best for you, the final goal is the success of your project, if you feel that passing predicate to generic methods will help make the code easily readable and/or maintainable, then there's no reason to worry if it's good practice or not.

And don't forget... It's still possible to make a good compromise between the two options.