5
\$\begingroup\$

I was wondering if someone could help me refactor this code. Although I know this looks bad, it works. Now I would like to refactor it so I would respect the open/closed principle in SOLID. Let's say the client wants a new search field to search data on, then I need to come to this method and add it in and this breaks the O/C principle. How would you guys change this?

public List<Customer> Search(Search searchData){
using(context)
{
    var query = context.Customers;

    if(!string.NullOrEmpty(searchData.FirstName)
        query.Where(c => c.FirstName == searchData.FirstName);

    if(!string.NullOrEmpty(searchData.LastName)
        query.Where(c => c.LastName == searchData.LastName);

    if(!string.NullOrEmpty(searchData.StreetName)
        query.Include(c => c.Address);
        query.Where(c => c.Address.StreetName == searchData.StreetName);

    if(!string.NullOrEmpty(searchData.PostCode)
        query.Include(c => c.City);
        query.Where(c => c.City.PostCode == searchData.PostCode);

    var customers = query.ToList();
    return customers;
}    }
\$\endgroup\$
10
\$\begingroup\$

Not sure if you know how dangerous omitting braces for if statements can be. Do you know what happens here :

if(!string.NullOrEmpty(searchData.StreetName)
    query.Include(c => c.Address);
    query.Where(c => c.Address.StreetName == searchData.StreetName);

no matter wether searchData.StreetName is null or not the query.Where(c => c.Address.StreetName == searchData.StreetName); will be executed nevertheless. I assume from the way you have formatted the code that you assume this will be only executed if the condition is true.

I would like to encourage you to always use braces {} although they might be optional. This helps you to structure your code which helps to make it more readable and by using them you make your code less error prone.

\$\endgroup\$
  • \$\begingroup\$ That is true and I allways use braces around if statements even if there is only 1 statment after the if condition, it looks like I forgot to add them here when I was typing the code... \$\endgroup\$ – pykos May 9 '16 at 5:41
3
\$\begingroup\$

Read Jon Skeet's article about open / closed.

In particular:

the description of Meyer’s approach to "make a change by extending a class" feels like a ghastly abuse of inheritance to me… and goes firmly against my (continued) belief in Josh Bloch’s advice of "design for inheritance or prohibit it"

Instead, interpret it as:

the phrase "closed with respect to X" means that clients are not affected if X changes.

The point of open / closed is that if you expect something to change, you should define an interface around it. In your case you could have searchProperty be an interface with an addToQuery(Query q) method and just do

forEach searchProperty: searchProperty.addToQuery(query);

but I think this is needlessly complicated because adding a new field doesn't break what users of the Search method expect.

If you had several predefined kinds of searches, like SearchWithHeightAndWeight and SearchWithSquareRootOfWaist in some imaginary clothing app, then it would make sense to have a Search interface and have each kind of Search be responsible for generating a query.

tl;dr the open-closed principle is really vague and trying to guess in advance which points of your program will vary is one of the hard things in software. what you should really be wary of is the possibility of making breaking changes to Search, which isn't the case here.

\$\endgroup\$
  • \$\begingroup\$ I agree that two or three search filters would not break any existing features or create an error, but what about when Customer class gets new features like a CreditCard class or some other feature, wich can have many properties, then I would need to add a substantial ammount of code to filter customer results, and this in turn results in a really long IF statement. I think that this kind of code can be rewriten to adhere to O/C principle. \$\endgroup\$ – pykos May 6 '16 at 11:35
1
\$\begingroup\$

To follow the open closed principle, I would suggest to change the Search object to a list of search conditions (as mentioned above from JonathanR).

Each SearchCondition class may implement its own query logic. e.g.:

public abstract class SearchCondition<TEntity>
{
    public abstract void AddToQuery(IQueryable<TEntity> query);
}

public class FirstNameSearchCondition : SearchCondition<Customer>
{
    private readonly string myFirstName;

    public FirstNameSearchCondition(string firstName)
    {
        myFirstName = firstName;
    }

    public override void AddToQuery(IQueryable<Customer> query)
    {
        query.Where(c => c.FirstName == myFirstName);
    }
}

// ...

public List<Customer> Search(IEnumerable<SearchCondition<Customer>> searchConditions)
{
    using (context)
    {
        var query = context.Customers;

        foreach (var searchCondition in searchConditions) {
            searchCondition.AddToQuery(query);
        }
        return query.ToList();
    }
}

Even if that is more code, I would prefer the solution above because:

  • You can add other search conditions without modifying existing code (OCP)
  • You can avoid the generic Search class with implicit state information (empty string or null means: "do not search for that property")
  • Each search condition logic is in its own class (SRP) (That seems to be oversized for one line of code, but search conditions may become more complicated)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.