Skip to content

Instantly share code, notes, and snippets.

@hagbarddenstore
Created November 10, 2017 13:56
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save hagbarddenstore/c4f93dd91b4a53b4322c9e4470ab91a8 to your computer and use it in GitHub Desktop.
Save hagbarddenstore/c4f93dd91b4a53b4322c9e4470ab91a8 to your computer and use it in GitHub Desktop.
[HttpPost]
public Model Get(int id)
{
var context = new TrineContext();
var investments = context.Investments.Where(i => i.Project_Id == id);
var projects = context.Projects.ToArray().Where(p => p.Id == id);
Expression<Func<Investment, bool>> c = i =>
i.Payment.State == PaymentState.Paid
|| i.Payment.State == PaymentState.PendingBankwire;
return projects.Select(p => new Model
{
Id = p.Id,
Amount = investments.Where(c).Sum(i => i.Amount),
Num = investments.Where(i => c.Invoke(i)).Count()
}).Single();
}
@hagbarddenstore
Copy link
Author

hagbarddenstore commented Nov 10, 2017

Line 1: The HTTP verb POST doesn't correspond with the action name which implies this is a GET action. The attribute should be dropped unless it was intended to be POST only. The action is a pure readonly action, so I recommend dropping the attribute.

Line 4: It's preferable if the EF context is injected into the controller and started / disposed at the start / end of the HTTP request. This allows multiple things in the HTTP request workflow to share the same EF context, thus allowing change tracking to work properly.

Line 5: Given that Project has a relation to Investments, it's better to fetch the project and pre-load the investments for that project than to issue another database query. Less roundtrips to the database is better.

Line 6: I don't think projects is the correct name, the code implies that only one project should be fetched. So, I would rename the variable to project. Calling .ToArray() tells EF to download the entire projects table, something that's not a very good idea. The best thing is to replace the entire line with this: var project = context.Projects.Include(p => p.Investments).FirstOrDefault(p => p.Id == id);. This will get a single project, eager load the investments, thus saving us a roundtrip to the database to fetch the investments.

Another possible twist on my line 6 suggestion is to write this:

var project = context.Projects.Select(p => new
    {
        p.Id,
        Amount = p.Investments.Where(i => i.Payment.State == PaymentState.Paid || i.Payment.State == PaymentState.PendingBankwire).Sum(i => i.Amount),
        Num = p.Investments.Count(i => i.Payment.State == PaymentState.Paid || i.Payment.State == PaymentState.PendingBankwire)
    }).FirstOrDefault(p => p.Id == id);

With this code we could summarize the action to:

[HttpPost]
public Model Get(int id)
{
    var context = new TrineContext();

    var project = context.Projects.Select(p => new Model
    {
        Id = p.Id,
        Amount = p.Investments.Where(i => i.Payment.State == PaymentState.Paid || i.Payment.State == PaymentState.PendingBankwire).Sum(i => i.Amount),
        Num = p.Investments.Count(i => i.Payment.State == PaymentState.Paid || i.Payment.State == PaymentState.PendingBankwire)
    }).FirstOrDefault(p => p.Id == id);

    if (project == null)
    {
        return NotFound();
    }

    return project;
}

I'm not sure how well EF will interpret a lambda expression, but it might be worth trying to break out the payment state check as in the original example to improve readability.

Number of issues I can spot: 11
1: Mistake or invalid use of the HttpPostAttribute. (Line 1)
2: Inline creation of an EF context. (Line 4)
3: Not using eager loading of Investments. (Line 5/6)
4: Variable name in plural when singular was intended. (Line 6)
5: Calling .ToArray() will download the entire table. (Line 6)
6: Using .Where() instead of FirstOrDefault when a single object is wanted. (Line 6)
7: Not aggregating in the original query will cause multiple database roundtrips. (Line 15)
8: Calling lambda expression rather than passing it, causing EF to crash or download the entire investments table to run everything in-memory. (Line 16)
9: Not using the overload of .Count() that accepts an expression. (Line 15)
10: Calling .Single() without handling the exception it might throw when there's 0 or more than 1 results returned.
11: Not returning a 404 Not Found, if .FirstOrDefault() was used rather than .Single(), a simple null check could be used to return the correct status code via .NotFound().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment