8
\$\begingroup\$

I have a DAL class full of LINQ queries which I would like to improve as I found that I'm repeating myself couple of times.

As you can see the part below is repeated couple of times and I was wondering if it is possible to reduce the number of repetitions (passing the query somehow?) and/or improve the queries overall.

.Where(x => x.Active == true && x.Variable.Active == true && x.Variable.Question.Active == true)

    private DashboardEntities db;

    public AnswerSelectLabelRepository(DashboardEntities _db)
    {
        this.db = _db;
    }

    public AnswerSelectLabel GetById(string idAnswerSelectLabel)
    {
        return db.AnswerSelectLabel.Find(idAnswerSelectLabel);
    }

    public IEnumerable<AnswerSelectLabel> GetAll()
    {
        return db.AnswerSelectLabel
            .Where(x => x.Active == true && x.Variable.Active == true && x.Variable.Question.Active == true)
            .OrderBy(x => x.DisplayOrder)
            .ThenBy(x => x.Value);
    }

    public IEnumerable<AnswerSelectLabel> GetByVariable(string idVariable)
    {
        return GetAll().Where(x => x.idVariable == idVariable);
    }

    public IEnumerable<AnswerSelectLabel> GetByVariable(IEnumerable<Respondent> respondents, string idVariable)
    {
        return respondents.SelectMany(x => x.AnswerSelect)
            .Select(x => x.AnswerSelectLabel)
            .Where(x => x.Active == true && x.Variable.Active == true && x.Variable.Question.Active == true)
            .Where(x => x.idVariable == idVariable);
    }

    public IEnumerable<AnswerSelectLabel> GetByQuestion(string idQuestion)
    {
        return GetAll().Where(x => x.Variable.idQuestion == idQuestion);
    }

    public IEnumerable<AnswerSelectLabel> GetByQuestion(IEnumerable<Respondent> respondents, string idQuestion)
    {
        return respondents.SelectMany(x => x.AnswerSelect)
            .Select(x => x.AnswerSelectLabel)
            .Where(x => x.Active == true && x.Variable.Active == true && x.Variable.Question.Active == true)
            .Where(x => x.Variable.idQuestion == idQuestion);
    }
\$\endgroup\$
10
\$\begingroup\$

The first thing (and this is very minor) is that the expression a == true and simply a are identical. Thus you don't need all those == true's.

The simplest, most obvious way is to simply extract a private method for your filter:

private boolean isActive(AnswerSelectLabel answer)
{
  return answer.Active
      && answer.Variable.Active 
      && answer.Variable.Question.Active
}

public IEnumerable<AnswerSelectLabel> GetAll()
{
  return db.AnswerSelectLabel
      .Where(isActive) //you'll pardon me if this is syntax error, I've been in java land too long now, where this would be this::isActive
      .OrderBy(x => x.DisplayOrder)
      .ThenBy(x => x.Value);
}

public IEnumerable<AnswerSelectLabel> GetByVariable(IEnumerable<Respondent> respondents, string idVariable)
{
  return respondents.SelectMany(x => x.AnswerSelect)
      .Select(x => x.AnswerSelectLabel)
      .Where(isActive)
      .Where(x => x.idVariable == idVariable);
}

and so on.

Slightly more sophisticated (and almost certainly overkill), is to extract some kind of interface on your answer, variable, and Question, classes. Something like

interface IActivatable
{
  public Active { get; }
}

If you make this change, you can start to play games with exactly what you're asking to be active:

public IEnumerable<AnswerSelectLabel> GetByVariable(IEnumerable<Respondent> respondents, string idVariable)
{
  return respondents.SelectMany(x => x.AnswerSelect)
    .Select(x => x.AnswerSelectLabel)
    .Where(x => new IActivatable[]{x, x.Answer, x.Answer.Question}.all(x => x.Active))
    .Where(x => x.idVariable == idVariable);
}

which, again, we can extract a method for. But this time we're parameterized on the actual things we want to check, which is nice because it means we can pick-and-choose what we want to be active. In your case it looks like this is always answer, question, and variable, but if that's not the case this is likely helpful:

public IEnumerable<AnswerSelectLabel> GetByVariable(IEnumerable<Respondent> respondents, string idVariable)
{
  return respondents.SelectMany(x => x.AnswerSelect)
    .Select(x => x.AnswerSelectLabel)
    .Where(x => isActive(x, x.Answer, x.Answer.Question))
    .Where(x => x.idVariable == idVariable);
}

private boolean isActive(params IActivateable activatables){
  return activatables.all(x => x.Active);
}

Of course, if this isn't the case, this is just useless speculative code, so don't do it.


If I could make a couple more notes:

  • using strings as a catch-all for ID's has gotten me in trouble, so now I push to create specialized ID classes (where a simple GUID wont do). Don't let this burn you too!
  • GetByQuestion(IEnumerable<Respondent> respondents, string idQuestion) and GetByVariable(IEnumerable<Respondent> respondents, string idVariable) are suffering from a sort of linq-feature-envy. They don't use your db field at all, so why are they on this class? Do these methods exist so that I can do something like this:

    var respondents = dal.GetAllRespondents(); respondents = respondents.Where(callerSpecificFilter); var asnwers = GetByVariable(respondents, "1234");

If this is the case, and you can afford the potential performance hit, you can remove the respondents parameter and take a Func<Respondent, Boolean> filter instead. Then simply apply the filter yourself.

\$\endgroup\$
  • 5
    \$\begingroup\$ Welcome to Code Review, great first post! \$\endgroup\$ – SuperBiasedMan Nov 26 '15 at 10:21
  • 1
    \$\begingroup\$ Thank you for the exhaustive and excellent answer! I went in the direction of your first solution as what I'm doing is quite simple. Regarding your last couple of notes. 1) I'm using sequentially generated GUIDs so I hope I won't get burned :) (though, sometimes it's hard to debug with values like that) 2) You are right about GetByQuestion and GetByVariable methods as you can't do the query you asked about. The reason why they are there is that I'm having hard times (and not enough experience) trying to organize the classes (especially business logic) as this is an MVC project. \$\endgroup\$ – wegelagerer Nov 26 '15 at 10:38
  • 4
    \$\begingroup\$ a == true and simply a are identical, - only if a isn't nullable. \$\endgroup\$ – Nick Udell Nov 26 '15 at 10:59
10
\$\begingroup\$

Extension methods to the rescue!

internal static class AnswerSelectLabelPredicates
{
    internal static IQueryable<AnswerSelectLabel> WhichAreActive(this IQueryable<AnswerSelectLabel> labels)
    {
        return labels.Where(x => x.Active && x.Variable.Active && x.Variable.Question.Active);
    }
}

I've chosen IQueryable over IEnumerable as it looks like you're using entity framework. If you're not, feel free to use IEnumerable again.

Then your queries look nice:

 public IEnumerable<AnswerSelectLabel> GetAll()
{
    return db.AnswerSelectLabel
        .WhichAreActive()
        .OrderBy(x => x.DisplayOrder)
        .ThenBy(x => x.Value);
}
\$\endgroup\$
  • \$\begingroup\$ You are right regarding the Entity Framework - definitely a thing to change! And as for solution goes, I think this will be surely useful for me in a bit more complicated queries. Thank you! \$\endgroup\$ – wegelagerer Nov 26 '15 at 10:43
  • 1
    \$\begingroup\$ @Agramer - I'll comment on the right post this time... stackoverflow.com/a/2876655/1402923 \$\endgroup\$ – RobH Nov 26 '15 at 11:36
  • \$\begingroup\$ Thanks for even more detailed info. Time to convert some queries :) \$\endgroup\$ – wegelagerer Nov 26 '15 at 12:01
4
\$\begingroup\$

You can refactor this predicate out to a method that checks if it matches your criteria.

private bool IsActive(AnswerSelectLabel answerSelectLabel)
{
    return answerSelectLabel.Active == true && answerSelectLabel.Variable.Active == true && answerSelectLabel.Variable.Question.Active == true;
}

Then your queries become:

public IEnumerable<AnswerSelectLabel> GetAll()
{
    return db.AnswerSelectLabel
        .Where(IsActive)
        .OrderBy(x => x.DisplayOrder)
        .ThenBy(x => x.Value);
}

On another note, though, I am making an assumption that these .Active properties are nullable, otherwise the == true portions are redundant and can be removed.

private bool IsActive(AnswerSelectLabel answerSelectLabel)
{
    return answerSelectLabel.Active && answerSelectLabel.Variable.Active && answerSelectLabel.Variable.Question.Active;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the solution! That was the way to go! I picked a bit more detailed answer. \$\endgroup\$ – wegelagerer Nov 26 '15 at 10:39

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.