An assumption: you never ever want to return a 500 HTTP status code (Internal server error). It may leak information you do not want to expose and it's not informative enough. Do not throw any exception: return the correct HTTP status code and eventually supplement it with the an appropriate error message. For example if
SaveAll() failed because a concurrent request you may return 409 (Conflict). This leads to two more points:
- In your code you should almost never catch
Exception and never throw it. There are many, more informative, exceptions to use which will play well with your exception handling policy (do you have one, right?).
SaveAll() returning a boolean just to throw an exception is, TBH, sub-optimal. If it's an error condition then it should be an exception (and caller MUST handle it, if it can). A boolean return value can too easily be ignored (and you don't want this to happen if data has not been saved).
You're not catching any exception, if anything happens with the repository then API calls will fail with an internal server error.
You parse the current user ID in many functions, move it out to a separate function. Also do not use
Int32.Parse() without specifying the culture you want to use (probably
Do not use
UserParams both in your repository and in your controller. It smells because:
- You can't change them independently.
- You have unused values you can specify in the query.
- If you add a new field and you forget to handle it in a controller method you might end up with a vulnerability.
ALWAYS validate and translate request parameters (and the easiest way is to have two separate objects). For example
userParams.Gender is directly exposed to your repository as an unvalidated string.
LikeUser()) does not actually need the user ID as parameter. Do not require useless and redundant data from the client (which must be validated) when you already have it server-side.
Do not compare strings using
== unless you exactly know what it means. In this case an even simpler and faster ordinal comparison is enough however...
Do not store the gender as a string. If your app is limited to binary genders (at birth) then you may use an
Gender is much more complex than a simple binary identity:
- Gender at birth is not necessarily binary.
- Gender at birth may not be the current gender.
- The assigned gender at birth may not be what a person identifies with.
- Especially in a dating app it's also important the sexual orientation (which is obviously independent from gender).
In short: do not use an hard-coded list and do not assume gender equals sexual orientation.
If your app will be successful then you definitely want to avoid to search the
likes table (which will grow much faster than the
users table) for an entry and to retrieve it only to return Bad request. Best case scenario is to have a simpler function to determine if the entry exists instead of reading, transmitting and mapping the entry itself. See also the next section...
I'm generally against the indiscriminate use of the Repository pattern. It's an invaluable tool but the price you pay in complexity to use it (properly) must be justified. I do not see the big picture here but you should seriously consider if directly using your ORM is enough. From what I see you really need a domain model, much more than a repository.
You have an instance field
_repo, you do not show that code but I do not see any synchronization mechanism and I do not see how you dispose its resources when controller is re-created. Generally, in your controllers, you should avoid instance fields as much as possible.
Your API should never ever work directly with the repository to perform any business logic. It should interact with an high-level model (or a Service layer on the top of the Data layer exposed by your ORM). For example I'd imagine the Like feature like this:
public async Task<IActionResult> LikeUser(int recipientId)
using (var service = _serviceFactory.Create())
await service.AddLike(GetCurrentUserId(), recipientId);
Yes, controller methods should not contain much more than this. All the logic must be TESTABLE then it is much easier to have it elsewhere. Note that in this fictional example I used a factory to create the service object: you may get it directly from your DI framework in the controller ctor but do not create it here (...
new MyService()) because you want to test the controller with a mocked service.
Note that the
AddLike() is probably implemented with a stored procedure, you really need performance here. Given that user should not be able to click Like for someone he already liked then when it happens it's definitely an error condition and as such it should trigger an exception. Code is probably more complex than this because of error handling but I think you now got the point:
catch (ItemNotFoundException e)
catch (ConcurrencyException e)
// TODO: try again?
If you structure well your service layer then you can probably move all this boilerplate code into a function method and reuse it in all controller methods.