8
\$\begingroup\$

The class I'm using is called Review and looks like this:

public class Review
{
    public virtual Guid Id { get; set; }

    public virtual int Rating { get; set; }

    public virtual Guid RestaurantId { get; set; }

    public virtual Guid UserId { get; set; }
}

I'm using a LINQ statement to first check a few things and afterwards group by a user's reviews. I'll explain what I mean with this:

var reviewsByUsers = 
    reviews
    .GroupBy(x => x.RestaurantId)
    .Where(x => x.Count() >= 3)
    .SelectMany(x => x.ToList())
    .Distinct()
    .GroupBy(x => x.UserId)
    .Where(x => x.Count() >= 2)
    .Select(x => x.ToList())
    .ToList();

And now step by step:

1. reviews is a list of all reviews given on a bunch of restaurants. This is not the database, it's a list I receive as a parameter of the method.

2. .GroupBy(x => x.RestaurantId) Group these reviews per each restaurant. This is for counting them later.

3. .Where(x => x.Count() >= 3) Exclude any restaurants which don't have 3 or more reviews on them.

4. .SelectMany(x => x.ToList()) Flatten the list so we can group again.

5. .Distinct() To be sure that I'm not getting doubles.

6. .GroupBy(x => x.UserId) Group the reviews per user. This is for counting them later.

7. .Where(x => x.Count() >= 2) Exclude any user which has less than 2 reviews.

8. .Select(x => x.ToList()) I don't need anything else per user, a list of reviews per user is enough.

All of this will result in a List<List<Review>> which is a list that contains a list of all reviews grouped by the UserId.

I'm certain there has to be a better way to perform these checks and create a list of lists grouped by user id. I'm just very new to C#, so I don't know how to increase this LINQ statement.

\$\endgroup\$
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Ethan Bierlein Jan 5 '17 at 14:56
  • \$\begingroup\$ Oh I see, sorry about that. \$\endgroup\$ – RandomStranger Jan 5 '17 at 14:57
  • \$\begingroup\$ Firstly. If reviews is the database context then you need to simplify that query a bit because its going to brick your SQL connection once you get some data in the tables. First build the DB query LINQ in a Repository then in the Class do all your grouping, filtering etc etc. IK can see several ToList() there on one query.. that is asking for big trouble! \$\endgroup\$ – Piotr Kula Jan 5 '17 at 15:10
  • 1
    \$\begingroup\$ @ppumkin it isn't a database context, I have clarified this in my post now. It's a list I receive through a parameter in my method. I have deleted some of my .ToList()s, is there more room for improvement? \$\endgroup\$ – RandomStranger Jan 5 '17 at 15:13
  • 1
    \$\begingroup\$ @RandomStranger exacly, without two groupings the results are invalid. \$\endgroup\$ – t3chb0t Jan 5 '17 at 15:28
6
\$\begingroup\$

There is actaully another solution that utilizes the Except extension. This means you first select all reviews with at least two users but remove those where the restaurant does not have at least three reviews.

This requires to write a new IEqualityComparer that I find is an overkill for this but on the other hand the Except clearly tells what kind of results we don't want to have. Anyway, the alternative solution:

reviews
    .GroupBy(r => r.UserId)
    .Where(g => g.Count() > 1)
    .Except(
        reviews
            .GroupBy(r => r.RestaurantId)
            .Where(g => g.Count() < 3),
        new ReviewComparer()
    );

where

class ReviewComparer : IEqualityComparer<IGrouping<Guid, Review>>
{
    public bool Equals(IGrouping<Guid, Review> left, IGrouping<Guid, Review> right) 
    {
        // left - user-group
        // right - restaurant-group
        // exclude the user group 
        // if it contains any restaurant-id of the excluded restaurant-ids
        return left.Any(x => right.Any(y => y.Id == x.Id));
    }

    public int GetHashCode(IGrouping<Guid, Review> reviews)
    {
        return 0; // treat all groups equally
    }
}
\$\endgroup\$
  • \$\begingroup\$ Edit: changed all ints to Guids, works now! Thank you! \$\endgroup\$ – RandomStranger Jan 5 '17 at 15:51
  • \$\begingroup\$ @RandomStranger oh, I took an int instead of the the Guid for testing, you need to change the int everywhere into a Guid or copy the corrected version \$\endgroup\$ – t3chb0t Jan 5 '17 at 15:55
3
\$\begingroup\$

First, stop using x as an all purpose variable name. It makes your query nearly impossible to follow. Using lambdas does not excuse us from using meaningful variable names.

Next, take note that SelectMany takes an IEnumerable, so you don't have to materialize the query by calling ToList(). You should be able to simply call it on x. Which in turn makes me wonder what exactly you're trying to flatten here.

.SelectMany(x => x) ???

Something doesn't quite sit right there. Typically, you'd call SelectMany on the property of an item in your enumerable, like in the doc I linked to above.

On the same note, this call is completely useless.

.Select(x => x.ToList())

You're returning an enumerable by materializing an enumerable to a list and then passing it back as an enumerable....

The big take away is that calling ToList() will execute your query, meaning that you're doing a lot of work in memory that you should be deferring to the database. I would highly recommend either writing a stored procedure, or using query syntax to write complicated queries like this. Let the database do what it's good at.

\$\endgroup\$
  • 3
    \$\begingroup\$ He flattens the very first groups. A group is enumerable and that's why there is no property but the instance itself. \$\endgroup\$ – t3chb0t Jan 5 '17 at 14:46
  • \$\begingroup\$ Well the reviews list is not a database, but a list I get from a parameter in my method. Is there more room for optimization on the counts? I can't shake the feeling that I'm doing this badly.. \$\endgroup\$ – RandomStranger Jan 5 '17 at 14:53

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.