Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Writing my first ASP.NET Web API web service. Still learing the power of Linq as well. I feel like there is a more ideomatic way of doing this:

public class StylesController : ApiController
{
    private Entities db = new Entities();

    // GET api/Styles
    public IEnumerable<Style> GetStyles(int division = 0, int family = 0, int model = 0, int year = 0, string market = "ANY")
    {
        market = market.ToUpper(); // make sure market is upper case

        IEnumerable<Style> q = db.Styles;

        if (division > 0) { q = q.Where(s => s.DivisionId == division); }
        if (family > 0) { q = q.Where(s => s.FamilyId == family); }
        if (model > 0) { q = q.Where(s => s.ModelId == model); }
        if (year > 0) { q = q.Where(s => s.Year == year); }
        if (market != "ANY") { q = q.Where(s => s.MarketCode == market); }

        return q.Distinct().OrderBy(s => s.Id).ToList();
    }
}
share|improve this question
From what I can see of this code, you are doing an AND operation. i.e. division = 1 AND year = 2012. Am I correct? – Jeff Vanzella Oct 10 '12 at 17:20
Correct, but each part of the AND is optional. – bejumi Oct 10 '12 at 19:55
Fine to return an IEnumerable but that first declaration should be IQueryable<Style> q = – Turnkey Oct 11 '12 at 23:29

1 Answer

up vote 2 down vote accepted

Good layout and white space.

Although the logic works, I think it is very confusing, and not overly obvious. At first glance, it the if statements are very confusing. I would also rename the variables to have an Id or Code to the end of them to keep naming consistent.

I think this is a good place where you could incorporate the Specification Pattern.

public class StylesController : ApiController
{
    private Entities db = new Entities();

    public IEnumerable<Style> GetStyles(int divisionId = 0, int familyId = 0, int modelId = 0, int year = 0, string marketCode = "ANY")
    {
        var divisionIsAMatch = new DivisionIsAMatchSpecification(divisionId);
        var familyIsAMatch = new FamilyIsAMatchSpecification(familyId);
        var modelIsAMatch = new ModelIsAMatchSpecification(modelId);
        var yearIsAMatch = new YearIsAMatchSpecification(year);
        var marketIsAMatch = new MarketIsAMatchSpecification(marketCode);

        var q = db.Styles

        return q.Where(
                divisionIsAMatch
                .And(familyIsAMatch)
                .And(modelIsAMatch)
                .And(yearIsAMatch)
                .And(marketIsAMatch).IsSatisfiedBy
            ).Distinct()
           .OrderBy(s => s.Id);
    }
}

Where

internal class DivisionIsAMatchSpecification : Specification<Style>
{
    private readonly int _divisionId;

    public DivisionIsAMatchSpecification(int divisionId)
    {
        _divisionId = divisionId;
    }

    public bool IsSatisfiedBy(Style style)
    {
        if (_divisionId == 0) 
        {
            return true;
        }

        return style.DivisionId == _divisionId;
    }
}

Just rinse and repeat for each of the specifications you need. Of course, you will have to create the base classes for them too.

I haven't tested this, but I have used this pattern before, and it works really nicely.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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