Code review and guidance
Trying to learn how to properly organise and built dotnet apps have some experience though a internship want feedback and how to be a better code report here: https://github.com/UmbrellaCrow612/PoliceCaseManagement the main areas i have worked on so far are the Identity system and some CAPTCHA stuff
GitHub
GitHub - UmbrellaCrow612/PoliceCaseManagement: Hub and Spoke patter...
Hub and Spoke pattern for PCMS. Contribute to UmbrellaCrow612/PoliceCaseManagement development by creating an account on GitHub.
44 Replies
Jesus Christ
You have projects with single classes/interfaces with them?
That's trying to outenterprise the enterprise practices lmao
Did you implement your own captcha...?
I did some basic captcha
If you look at the captcha project
Most advanced one I'm working on is carousel captcha
But got audio captcha and math so far @ZZZZZZZZZZZZZZZZZZZZZZZZZ
Can't say I understand this choice, but cool
Am I over doing it ?
Or is this or industry would do it @ZZZZZZZZZZZZZZZZZZZZZZZZZ
I have never seen anybody implement their own captcha
With ReCaptcha, HCaptcha, Turnstile, and so on existing
Good for a learning exercise, for sure
Not for real-life usage
Is there anything specific you'd like feedback to focus on first here?
Also is there a reason you posted this in #help rather than #code-review ?
Sorry first time here but would like feedback so structure of projects if necessary or not following best practice and mainly how I'm doing identity with the identity project as that's where most the work is done so far @Rory
So "Best practice" is always a funny one because best practice... is contextual. Sometimes simpler is better, sometimes you need to organise in a specific way (e.g. for framework targets). There's a lot of good articles out there with different schools of thought on project organisation.
What I'd say at a glance is that while you don't have much here it's probably better to start simpler and smaller and start to break things out into concrete areas as you build them out and evolve them. That's not to say that thinking ahead and drafting a plan is a bad thing, but this solution is pretty overwhelming because there's 35 projects and not a lot within.
Particularly because projects aren't just for logical organisation but for building different assemblies.
Also, best practices will be different for a greenfield modern application, and different for an enterprise application with a dinosaur for a lead developer
In the former case, you'd use VSA architecture. In the latter, DDD all the way
And so on
Ok so I should try to narrow down project size
Also namespaces exist for a reason! They can serve as great logical ways to organise rather than breaking things apart. Let's say you used namespaces named after your domain concepts here and organised into folders instead you would probably have started with an api project something like
BUT I am making an assumption that your goal would be to host all of this in one server? If you're actually trying to bust this up into lots of different deployed services, then having different deployable units makes more sense
Am I flowing DDD properly my main idea was to break everything Into different applications / microserves in a sense hosted on different servers that communicate ina hub and spoke way without them talking to each other
Where the front end would be the hub
Also generally it's typical/common for tests and source in dotnet spaces to be organised differently at the root in two folders. Something like:
In DDD, you break by layer
Usually Data -> Repository -> Service -> Main
You have a project that has a single
IMapper interface
That's beyond DDD lmaoDDD is a whole other can of worms and not necessarily microservices.
It's a very misunderstood topic I think in industry lol, so easy to follow bad guidance
DDD is, like, the opposite of microservices
VSA is closer to microservices tbh
Lamo I thought it was like my Auto mapper for that one project
But to that end you would want to shoot for organising into services that focus on a discrete domain. At that point you'd maybe even have each concept broken out into its own solution/repo
Am I doing that currently I have areas like identity into its own API that I would use in a sense ?
So if this is a distributed system, having identity (a platform concern, not a thing to care about as a domain in a DDD sense) in its own deployable unit can be quite a good thing because you don't have to dilute the code that solves your actual business concerns with something technical like that.
Sure, everything will consume identity/auth but ideally nothing more detailed than validating auth policies and such. The nitty gritty of how that is setup is to the side
I'm struggling to understand why
Entity and CaseManagement were broken into two separate things since the readme suggests direct relationThe README is a bit out of date I should say, my rational at the beginning was to separate case stuff Into its own service, people / entities into its own, identity into its own and document / evidence into its own API,s
Also something you've done here which you will absolutely find a LOT in industry is organise your code into folders/namespaces by feature of the code (interfaces, models, dtos). Personally I don't like that lol, but it is a common way to go
Then a front end to be the hub
Some code thoughts (I am jumping around randomly)
Your
Identity.Core.Models.Application class has something that stands out to me - it looks to be a very open property bag (public setters, no constructor to constrain how it is created, public collections you can add to) but you have a specific methods around controlling how the department is used - both overwriting it and presenting a fact based on the value IsLinkedToADepartment - any reason you didn't make DepartmentId private? Also given there is a public Department it seems like these two properties have no guarantee of being consistent with each other, right?
In fact I see that sort of thing elsewhere - EmailVerificationAttempt has IsSuccessful and UsedAt which the class looks like it tries to control partially, but the properties are still open to the public.I tried to have state like that be co troller through methods I made them public because I thought I had to for ef core to pick them up and persist them ?
I guess it should be private so it can't be changed forcefully
ah so you're using the same models for persistence and behaviour then and you want EF to directly load/save those objects
Yh there basically my tables
So that'll work but it can be a bit smelly. What you then have is effectively a leak of your persistence model through your domain - the implementation detail of the database is visible throughout the code a bit, and the two models can't necessarily evolve independently (or can't evolve easily)
Should I have a class for my table and another for business logic rules like
IsValid etcIt depends because that does also overcomplicate it in ways (especially if your application is kinda CRUD by nature), but I do prefer that sort of thing. I aren't too familiar with EF so can't advise any patterns specific to that but...
When I do this I tend to have a repository which might internally map a domain model to and from a persistence model, then use that for the actual save/load.
But I'm sure EF can do something a bit funky for you here, I just dunno what
I could look I to ef core to see how I could hide those fields but have them persisted
For EF, the general rule is to keep the models anemic
Just classes with properties
Exact representations of the tables
Then you map them between layers
yeah so your literal thinnest property bags possible used within EF and you convert between them yourself?
ye
By yourself, or with Riok.Mapperly, Mapster, Automapper, whatever
Another comment about your code: I opened up
AuthenticationController and the constructor is massive. Those controller methods are doing quite a lot - it might be more sane to stick to strict API handling and try to pass the request onto some other classes that are more designed to handle the actual actions you're trying to performYh the authication co troller is the thickest out of all of them
Authentication
I don't want to make a application project or something like that because I feel like there's no point I'd rather talk to a dB store and stuff in the API endpoint instead of hiding it ?
In layered architecture like this, it's services that would be doing all that business logic
Controller calls a service, service calls the repository, repository calls EF
It's a bullshit useless overcomplication, but what can you do, enterprise being enterprise
You basically said what I was gonna lol. There's better ways to do this but
Controller/Service/Repository is at least a sane start because you push your API handling and Database handling to the edges and you put your "business code" cleanly outside of those
and not "sane start" in "this is for beginners" but in "a lot of industry does this and it is at the very least predictable and manageable"Would services be in a different project then
Like identity.application and /services
I am begging you to use less projects lol
Oh ok 😅
The way I'd start a new api is probably with just a single project! And then as you need to share code and host it in new ways, think about breaking it out.
That's not to say you wouldn't organise code within the project into logical areas but still
Back to the auth controller: breaking the controller up may also be an option but lets' make some points about
Login :
- It's 90 lines long, that could be considered quite long for a controller function (though I am sad to say that's not uncommon at all in industry)
- You get the IP address before you use it and, actually, maybe never use it because you can return a response before its used.
- You do two if statements to find a user. It might be neater to implement this as one method userManager.FindUser(string username, string? Email) since the API behaviour is the same. This could result in just a single DB query if that's how users are stored rather than multiple hits.
- You look up a user by email even if the email has not been provided. Can that ever succeed? Needless DB hit
- It is not possible to hit the if user is null check on line 71
- You then run through a list of authorisation checks - password correct, user lockout, email confirmed, phone confirmed. all these might logically belong in a single service call, let's say a method like public LoginResult TryLogin(LoginAttempt attempt, LoginRequest request) on a class AuthService. You could then capture all your different login checks and return a success OR a failure type to the controller, which is purely responsible for transforming that into an HTTP response then!
- Similar above with the device checks - that might live in the same auth service call or a separate oneOk makes sense
Ayt thanks for help will add feedback to the project over time
@ZZZZZZZZZZZZZZZZZZZZZZZZZ and @Rory
this is also a good point to keep in mind but not to say "don't do it here" - Identity and CAPTCHA are very specific topics in no way related to your core domain here. Strategic DDD would suggest bringing in external tooling for these. You are doing that a lot with identity by leaning into the identity framework stuff though.
and no prob, happy to help
Finally on that login method: You have so much untested code that you can only write API tests for. API tests aren't necessarily bad, but you might have a much nicer time writing tests against
AuthService.Login for the majority of validating your auth logic without having to care about the http specifics atop that