CODEWITHSUNDEEP
c#oop
tfiwsrets
tfiwsrets

Reputation: 87

Merge two methods into one

How can i merge methods below? and should i actually do this?

public IQueryable<ItemDTO> RepGetMonthItems(string inOut, string planFact, int month)
{
    return GetItemsWithCategory().
            Where(i => i.InOut.Equals(inOut)).
            Where(i => i.PlanFact.Equals(planFact)).
            Where(i => i.DateTime.Month.Equals(month));
}

public IQueryable<ItemDTO> RepGetYearItems(string inOut, string planFact, int year)
{
    return GetItemsWithCategory().
            Where(i => i.InOut.Equals(inOut)).
            Where(i => i.PlanFact.Equals(planFact)).
            Where(i => i.DateTime.Year.Equals(year));
}

Upvotes: 3

Views: 1609

Answers (5)

Baldrick
Baldrick

Reputation: 11840

I agree with the 'do one thing' principle. But I also believe in DRY.

So.. extension methods to the rescue!

Have the main function do one thing (get report items) and extension methods filter by year or month.

Declare an extension method class like this:

    public static class QueryExtensions
    {
        public static IQueryable<ItemDTO> ForYear(this IQueryable<ItemDTO> query, int year)
        {
            return query.Where(i => i.DateTime.Year.Equals(year));
        }

        public static IQueryable<ItemDTO> ForMonth(this IQueryable<ItemDTO> query, int month)
        {
            return query.Where(i => i.DateTime.Month.Equals(month));
        }
    }

The create a cut-down RepGetItems method like this:

    public IQueryable<ItemDTO> RepGetItems(string inOut, string planFact)
    {
        return GetItemsWithCategory().
            Where(i => i.InOut.Equals(inOut)).
            Where(i => i.PlanFact.Equals(planFact));
    }

Usage then looks like this:

 var yearResults = originalQuery.RepGetItems(input, fact).ForYear(2015);
 var monthResults = originalQuery.RepGetItems(input, fact).ForMonth(10);

or even:

 var yearMonthResults = originalQuery.RepGetItems(input, fact).ForYear(2015).ForMonth(10);

Total flexibility, no loss of 'single purpose' principle.

Upvotes: 2

Fabjan
Fabjan

Reputation: 13676

How about using optional parameter here:

public IQueryable<ItemDTO> RepGetMonthItems(string inOut, string planFact, int month, int year = -1)
    {
        return GetItemsWithCategory().Where(i => i.InOut.Equals(inOut) 
          && i.PlanFact.Equals(planFact)) && year == -1? 
            i.DateTime.Month.Equals(month) : i.DateTime.Year.Equals(year));
    }

Take a look at this example :

enter image description here

As you can see each time you call method Where() you make a lot of extra work.

Upvotes: 0

xanatos
xanatos

Reputation: 111820

The .Where conditions can be easily added to a query.

public IQueryable<ItemDTO> RepGetYearItems(string inOut, string planFact, int monthyear, bool useMonth)
{
    var query = GetItemsWithCategory().
        Where(i => i.InOut.Equals(inOut)).
        Where(i => i.PlanFact.Equals(planFact));

    if (useMonth)
    {
        query = query.Where(i => i.DateTime.Month.Equals(monthyear));
    }
    else
    {
        query = query.Where(i => i.DateTime.Year.Equals(monthyear));
    }

    return query;
}

or using int?

public IQueryable<ItemDTO> RepGetYearItems(string inOut, string planFact, int? year, int? month)
{
    var query = GetItemsWithCategory().
        Where(i => i.InOut.Equals(inOut)).
        Where(i => i.PlanFact.Equals(planFact));

    if (year != null)
    {
        query = query.Where(i => i.DateTime.Year.Equals(year.Value));
    }

    if (month != null)
    {
        query = query.Where(i => i.DateTime.Month.Equals(month.Value));
    }

    return query;
}

Note that this version is different from the one of @TimSchmelter... It isn't fixed (it depends on the "type" of SQL) what happens if you have in a query some "dead code" (pieces of query that aren't "active" because some parameters have a certain value, like int his query). His query clearly contains some "dead code" (the whole i => !year.HasValue || i.DateTime.Year.Equals(year.Value)). If possible I prefer to omit it.

Upvotes: 0

Tilman Hausherr
Tilman Hausherr

Reputation: 18851

No, you shouldn't. The three answers I see all break the "do one thing" rule by Robert "Uncle Bob" Martin. Read about it here.

Upvotes: 3

Tim Schmelter
Tim Schmelter

Reputation: 460028

Maybe you could use Nullable<int> for year and month:

public IQueryable<ItemDTO> RepGetItems(string inOut, string planFact, int? year, int? month)
{
    return GetItemsWithCategory().
        Where(i => i.InOut.Equals(inOut)).
        Where(i => i.PlanFact.Equals(planFact)).
        Where(i => !year.HasValue  || i.DateTime.Year.Equals(year.Value)).
        Where(i => !month.HasValue || i.DateTime.Month.Equals(month.Value));
}

Upvotes: 1

Related Questions