Created
November 10, 2017 13:56
-
-
Save hagbarddenstore/c4f93dd91b4a53b4322c9e4470ab91a8 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[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(); | |
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 toproject
. 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:
With this code we could summarize the action to:
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().