Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
var fpslist = db.FPSinformations.Where(x => x.Godown_Code != null && x.Godown_Code == godownid).ToList();
var data1 = fpslist.GroupBy(x => x.Ration_Card_Type1)
    .Select(x => new
    {
        CardType_Name = x.Key,
        CardType_Count = x.Sum(y => y.Ration_Card_Count1)
    }).ToList();
var data2 = fpslist.GroupBy(x => x.Ration_Card_Type2)
    .Select(x => new
    {
        CardType_Name = x.Key,
        CardType_Count = x.Sum(y => y.Ration_Card_Count2)
    }).ToList();
var data3 = fpslist.GroupBy(x => x.Ration_Card_Type3)
    .Select(x => new
    {
        CardType_Name = x.Key,
        CardType_Count = x.Sum(y => y.Ration_Card_Count3)
    }).ToList();
var data4 = fpslist.GroupBy(x => x.Ration_Card_Type4)
    .Select(x => new
    {
        CardType_Name = x.Key,
        CardType_Count = x.Sum(y => y.Ration_Card_Count4)
    }).ToList();
var data5 = fpslist.GroupBy(x => x.Ration_Card_Type5)
    .Select(x => new
    {
        CardType_Name = x.Key,
        CardType_Count = x.Sum(y => y.Ration_Card_Count5)
    }).ToList();

var GodownRCCount = data1.Where(x => x.CardType_Name != null).ToList();
var GodownRCCounts = GodownRCCount;
GodownRCCount = data2.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data3.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data4.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data5.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);

I have 10 columns in my database like this:

Ration_Card_Type1
Ration_card_count1
Ration_Card_Type2
Ration_card_count2
Ration_Card_Type3
Ration_card_count3
Ration_Card_Type4
Ration_card_count4
Ration_Card_Type5
Ration_card_count5

Now what I want is to get the sum of Ration_Card_Counts and its type from its type.

Expected output:

CardType_Name
CardType_Count

It works fine, but I want to optimize it in max possible way as this will be inside a loop.

share|improve this question
3  
Do you have any power to get the database changed? You're jumping through hoops because it's not been properly normalized. – RubberDuck Dec 14 '15 at 11:24
    
yes I have what will be your suggestion? – Arijit Mukherjee Dec 14 '15 at 11:25
    
You're "asking for advice about code not yet written", and such questions are off-topic. – BCdotWEB Dec 14 '15 at 13:35
1  
@BCdotWEB no i have already implemented the code above but wanted it to improve as well as was open to any db changes etc – Arijit Mukherjee Dec 15 '15 at 5:08

From what you gave us, there's nothing you can do to optimize your query except changing the way your data is stored.

Having Ration_Card_Count[1..5] is a big code smell. There's something wrong in the implementation.

Though, you could change this code to be a little less repetitive.

Most code is duplicated in there, so let's refactor it :

var data1 = fpslist.GroupBy(x => x.Ration_Card_Type1)
.Select(x => new
{
    CardType_Name = x.Key,
    CardType_Count = x.Sum(y => y.Ration_Card_Count1)
}).ToList();

This could be extracted in a method but beforehand, we need to create a small data type for your anonymous type because methods cannot return anonymous types.

struct CardGrouping
{
    public string Name { get; set; }
    public int Count { get; set; }
}

private static IEnumerable<CardGrouping> GetCardGrouping(IQueryable<??> queryable, Func<???, int> groupingFunction)
{
    return queryable.GroupBy(groupingFunction)
        .Select(x => new CardGrouping()
        {
            Name = x.Key,
            Count = x.Sum(groupingFunction)
        }).ToList();
}

Notice the ???, they're here because there's not enough context in your question, I can't figure what type to use. In your next question, please add more context! :)

Now, we've got a method that does the nasty work for us, let's see what the code looks like :

var data1 = GetCardGrouping(fpsList, y => y.Ration_Card_Count1);
var data2 = GetCardGrouping(fpsList, y => y.Ration_Card_Count2);
var data3 = GetCardGrouping(fpsList, y => y.Ration_Card_Count3);
var data4 = GetCardGrouping(fpsList, y => y.Ration_Card_Count4);
var data5 = GetCardGrouping(fpsList, y => y.Ration_Card_Count5);

var GodownRCCount = data1.Where(x => x.CardType_Name != null).ToList();
var GodownRCCounts = GodownRCCount;
GodownRCCount = data2.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data3.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data4.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);
GodownRCCount = data5.Where(x => x.CardType_Name != null).ToList();
GodownRCCounts.AddRange(GodownRCCount);

You repeat x => x.CardType_Name != null with every data, so maybe we should add this to our method :

private static IEnumerable<CardGrouping> GetCardGrouping(IQueryable<??> queryable, Func<??, int> groupingFunction)
{
    return queryable.GroupBy(groupingFunction)
            .Where(x => x.Key != null)
            .Select(x => new CardGrouping
            {
                Name = x.Key,
                Count = x.Sum(groupingFunction)
            }).ToList();
}

Now what does it look like :

var data1 = GetCardGrouping(fpsList, y => y.Ration_Card_Count1);
var data2 = GetCardGrouping(fpsList, y => y.Ration_Card_Count2);
var data3 = GetCardGrouping(fpsList, y => y.Ration_Card_Count3);
var data4 = GetCardGrouping(fpsList, y => y.Ration_Card_Count4);
var data5 = GetCardGrouping(fpsList, y => y.Ration_Card_Count5);

var GodownRCCounts = new List<???>();
GodownRCCounts.AddRange(data1);
GodownRCCounts.AddRange(data2);
GodownRCCounts.AddRange(data3);
GodownRCCounts.AddRange(data4);
GodownRCCounts.AddRange(data5);

The naming is pretty terrible. Snake_Case shouldn't be used in C#. If you can change all those Ration_Card_Count5 to RationCardCount5, it'd be great. Also, variable shouldn't be PascalCased, but camelCased. So GodownRCCounts = godownRCCounts.

Lastly, try to use var only when it's possible to guess the type used by reading the code. Right now it is impossible and it makes the code harder to read/review.

Edit : I don't think this code even works. Is Ration_Card_Count an int or a string? Or maybe a Nullable<int>? Because you use it in the Sum function, which takes an int, but you also check for null, which isn't a possible value for int. What does that mean?? If you check int == null, you can remove all the checks for that, it'll never happen.

share|improve this answer
1  
Ration_Card_Count is an int CardType_Name is being checked for Null and is a string What additional information do you need, sorry for not being descriptive my 1st time in codereview thanks for the small updates, most of the code is not mine, but i will surely try to change it – Arijit Mukherjee Dec 15 '15 at 5:15
    
You shouldn't check the int for null, because that can never happen! :) And It's alright, first time is hard for everyone. In your next question, try to put as much context as possible! :) – TopinFrassi Dec 15 '15 at 14:11
    
im checking the name for null not the int – Arijit Mukherjee Dec 15 '15 at 16:29
    
Yes but CardType_Name = x.Key and x.Key = y.Ration_Card_Count1. Unless there's something we're missing in here – TopinFrassi Dec 15 '15 at 16:31
    
no you are not missing anything but i dont in some cases of groupby null value was getting added in normal scnario the count should be 1 in every case but in few cases count was 2 n 2nd was null so added null case – Arijit Mukherjee Dec 15 '15 at 16:35

First and most obvious issue is usage of ToList(), it's not required and you force creation of multiple (and possibly big) lists moreover you're repeating almost same code again and again. Step by step:

var fpslist = db.FPSinformations.Where(x => x.Godown_Code == godownid);

Unless comparison operator for Godown_Code is broken then you shouldn't need to check for null (in case you may opt for ?. to avoid it). Also you don't need .ToList(), drop it. If you will execute such code inside a loop you may consider to move this outside the loop (to be sure more context is needed).

Now let's make a reusable function (and also avoid to create multiple anonymous types, it's a micro optimization but it also simplifies our code):

static IEnumerable<Tuple<TName, TCount>> CountByType<T, TName, TCount>(
    IEnumerable<T> list,
    Func<T, TName> nameSelector,
    Func<IEnumerable<T>, TCount> aggregator)
{
    return list.Where(x => Object.Equals(nameSelector(x), null) == false)
        .GroupBy(x => nameSelector(x))
        .Select(x => Tuple.Create(x.Key, aggregator(x)));
}

Now your code can be greatly simplified (I assumed you cannot change database columns otherwise everything can be even simpler, see last section of this answer). I also used generics because type of your columns is unknown.

    var data1 = CountByType(fpslist, 
        x => x.Ration_Card_Type1,
        x => x.Sum(y => y.Ration_Card_Count1));

    var data2 = CountByType(fpslist, 
        x => x.Ration_Card_Type2,
        x => x.Sum(y => y.Ration_Card_Count2));

   // Repeat for each set

   var result = data1
       .Concatenate(data2)
       .Concatenate(data3)
       .Concatenate(data4)
       .Concatenate(data5);

Note that from your code it seems you can have multiple rows with a value for Ration_Card_CountX. If you have just one row then all code will be greatly reduced. Here I used a Tuple<T1, T2> because I do not know exact types of your objects, you'd better use a proper class instead.

Note that you can consume result as-is (an enumeration) or materialize a list (don't forget this if data comes from DB and connection will be disposed, LINQ execution is deferred).


About DB

I do not know if you can change your database schema and I do not know exact layout of your data then this section may not apply in your case.

If your data is in the form:

Ration_Card_Type1   Ration_Card_Count1   Ration_Card_Type2   Ration_Card_Count2
Type1                                5   NULL                                 0
NULL                                 0   Type2                               11
Type1                               12   Type2                                3

Then you have to keep your actual code but I would suggest to do not map 1:1 column names with C# property names (conventions are pretty different) then Ration_Card_Type1 should be RationCardType1 (assuming you can't simply have Type1, Type2 and so on).

If, for example, you have data with this simplified layout:

Ration_Card_Type1   Ration_Card_Count1   Ration_Card_Type2   Ration_Card_Count2
Type1                                5   NULL                                 0
NULL                                 0   Type2                               11
Type1                               12   NULL                                 0

Then you may both optimize and simplify your DB (it will also impact code):

Type   Count
1          5
2         11
1         12

In that case all you need (assuming your C# entities maps 1:1 with DB columns):

var result = db.GroupBy(x => x.Type)
    .Select(x => new { Type = x.Key, Count = x.Sum(y => y.Count) });
share|improve this answer
    
I'm open to database changes as well – Arijit Mukherjee Dec 15 '15 at 5:09
    
On the same db row can you have a value for each tuple name & count? – Adriano Repetti Dec 15 '15 at 6:12
    
how are you saying can you show me an example? – Arijit Mukherjee Dec 15 '15 at 6:13
    
Ok, few hours to be in office! Breakfast time here :-) – Adriano Repetti Dec 15 '15 at 6:15
    
ok enjoy :) it's almost 12 noon here :) – Arijit Mukherjee Dec 15 '15 at 6:17

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.