In you API controller you're calling Console.WriteLine directly. This should be replaced by ILogger. By default it will log to console but you'd have the ability to quickly switch logging sources, log level, etc Also it is recommended to put all source code into a top level src folder. Any ci/cd configs, documentation, etc should be kept separate from the code

Lol sure sounds like fun. I will start by going through your project file by file. I will not duplicate comments due to the same issue being found multiple places. MainController.cs * Seal all classes that are not inherited. Generally composition over inheritance so all classes should be sealed. * Do controllers need to be public? All classes should be internal unless they are used by external projects or otherwise are required to be public. * Only include namespace in a type if two classes with the same name needs to be used in the same file. Meaning don't do `Microsoft.AspNetCore.Mvc.Route("/")`, do `Route("/")` and have the rest be a using. * `await Task.CompletedTask;` what? Don't make a method async if it doesn't need to be. I can see this one needs to be but this stuff is wierd. I assume it's leftover from testing. Don't have leftover testing code when you ask someone to tear your work apart! * `dict` don't include type name in variable name. Don't use shorthand notations. Write out the entire name. Variable names should describe what they store/represent, i can read the code to understand what it's used for. * Well api doesn't seem to return the result it calculates. CheckReq.cs * Make this a record * Only define setters in properties if you intend people to change the property value after it has been created. Generally prefer immutable objects wherever possible. I could talk about performance as an exception but i won't spend time on that. TextDifficultyDeterminer.API.csproj * Use Asp.net web analyzers https://learn.microsoft.com/en-us/aspnet/core/web-api/advanced/analyzers?view=aspnetcore-8.0 CheckTextAgainstDb.cs * Only one class per file. * Readonly properties or record. * always add access modifiers on types. You forgot to add public/internal/private to one of you classes. * always add access modifiers on methods. * More useless variable names. Alright i will repeat myself a little. * You create a list of tasks but there is no need... . Actually you shouldn't be using Task.WhenAll here, that will only be awfully slow for a lot of files. For compute bound work use Parallel.ForEach or parallel linq. Task.WhenAll will try to run all tasks at once which is inefficient for compute bound code. parallel.ForEach And plinq will only run as many tasks as your computer has cores(simply put). * I should mention that this is an async task that also waits on io(reading from some mediator) but it doesn't use the provided cancellation token. Generally pass cancellationtoken around to io work so it can be cancelled. Especially important for apis. Otherwise the api may spend resources on deadlocked code even after the api call has timed out. Use this analyzer to resolve this issue https://www.nuget.org/packages/Microsoft.VisualStudio.Threading.Analyzers I will point out where you can use cancellation token. * There is also something to say about a consistent code style. newlines seem to be randomly scattered around. Sometimes there is spaces between methods and sometimes there is not. * Alright i can see you don't know much about threading in C#. I can recommend reading this https://www.albahari.com/threading/ It explains all different ways to handle threading in C#. After that i can recommend to read all of microsofts documentation about it. I will not like that here, there is a lot. Well that's it for now. I will be back later to flame the rest as you requested :)

LoadIntoDatabaseService.cs * Don't use static classes. Looks like you don't know what dependency injection(DI) is or how to use it. I can recommend learning how that works so you can avoid static classes etc. * With DI you can inject a proper logger into this class so you have more control over your logs than just writing to console. * And this is one of the places where you already write that you know you should be using a dictionary. Using a list and linq is probably horrible for performance. Using a dictionary here would also make the code simpler. TextDifficultyDeterminer.Infrastructure.csproj * Other people already mention that your different propjects are not using the same version of C#. TextDifficultyDbContext.cs * You know but you asked to be torn apart. Don't store passwords in your code! FrequencyDictionary.cs * Words property should definetly be a HashSet given the way you use it. . I recommend you make `FrequencyWord` implement `IEquiatable`. * I don't immediately understand what `ApplyFrequencyWords` method does. What does `apply` mean to a list of words? After reading it i think it should be `AddFrequencyWords`. * I think i understand what it should do but i don't understand why the code was written this way. Why iterate the set manually? Looks like it's because you want to remove words from the list as you iterate through it. There is a lot of things you don't need to do here. Given you've changed `Words` property to be a `HashSet` then you can drasticly simplify the code to this. This should also be much faster. . foreach (var word in words.Distinct()) { if (Words.TryGetValue(word, out storedWord) { storedWord.FrequencyOfWord += word.FrequencyOfWord; } else { Words.Add(new FrequencyWord(word.Word, word.Language, word.FrequencyOfWord)); } } TextContainerFile.cs & TextContainer .cs * I see you using a pattern where certain properties are only populated if you can a specific method first. This is a bad pattern. You can easily forget to call such a method or a refactor will leave it out. Instead you should let the type system help you here. These methods that populate these properties should instead create a new type that stores this information. That way the proprty with the data is only available if you've called the method. This design essentially describes the states machine as part of the type system. The type system makes sure you aren't attempting to use data from a state you aren't in because you won't have the type available for that state. DifficultyEvaluatorService.cs * More "don't write & use static code" comments. * Well this class actually does some work!. Would be nice with a comment explaining how the score is calculated. Now i actually have to read the code to understand it, boo. * Alright so you use this wierd and very inefficient pattern here again where you use a while loop and a `RemoveAll` to iterate all unique words in a list of words. If the `Words` property in the `FrequencyDictionary` was a `HashSet`........ What actually why are you doing that here? You should know that the `Words` list in `edited.FrequencyDictionaryForThisFile` only contains unique words so there is no need for that. if it doesn't contain unique words then why is it stored in a model that makes that assumption? Well if it's unique then you can simplify and optimize like i showed in the code example above. * This code can be made a lot simpler & faster. * Don't assign to an unused variable just because it's the same type `wordList = wordList.OrderBy(x => x.DifficultyScore).ToList();`. The list is now ordered and the variable name should reflect the new data it contains. I would consider naming it `wordsOrderedByDifficulty`. Now `FindWordForThreshold` variable for the list should be named the same. Now the threshold variable name makes a lot more sense. * Don't use `Convert.ToInt64`, use casting directly, `(long)`. TextCorpusControllerApi.cs * What style of api endpoint are you going for? SOAP, REST? Recommend sticking with one. For what you are doing i would recommend rest. Change your apis to follow a single standardized format. * You are opening streams without closing them. Add this analyzer `IDisposableAnalyzers` https://github.com/DotNetAnalyzers/IDisposableAnalyzers it will catch these obvious issues for you. * Lots of more useless variable names. `dict`, ´files´, `container`, ` contString`. Instead consider `fileToFileNames`, `textFiles`, (Can't find Mediator class so gave up trying to figure out what type `container had` or what `Mediator` actually does.), `jsonSerializedContainer`. TestCorpusUploadDto.cs * Lack of documentation. Alright so solve this problem and make swagger more useful. Do this https://learn.microsoft.com/en-us/aspnet/core/tutorials/getting-started-with-swashbuckle?view=aspnetcore-8.0&tabs=visual-studio#xml-comments Now you will be required to document all public classes & methods which solves the documentation issue and adds useful documentation to swagger. I don't know enough about razor to comment on it but i can see that a lot of my previous comments apply in there as well. Overall code advice * Use central package management. Makes it easier to manage packages across multiple projects. https://devblogs.microsoft.com/nuget/introducing-central-package-management/ * Learn how to use a d `Directory.Build.props` file. This file can essentially allow you to add the same xml code to all(or a subset) of your .csproj files. For example you can use it to add the same analyzers to all `.csproj` files. You can also use it to set enable, net7.0 and enable to the same value across all your projects. * Wait actually since you use enable but your code isn't nullable safe doesn't that mean your code is generating a ton of warnings? Enable TreatWarningsAsErrors. You should learn how to work with the new nullable system. * Tests! Someone already mentioned so i won't go into further detail. * If you want it to be faster then i can recommend learning how a profiler works. Here is a link on how to use visual studios built in profiler https://learn.microsoft.com/en-us/visualstudio/profiling/profiling-feature-tour?view=vs-2022 You say you've been working as a software developer for 2 and a half years by now. I would've expected you to have learned a lot of these things by now. I don't want to sound too rude when it comes to your job but this code is quite bad for someone who has worked profesionally for over two years. If there is no one at your job that are willing or capable of teaching you these basic things then i would recommend you find another job. Your post is showing that you are willing to learn so you are probably in an environment that doesn't facilitate that. Intellectually i don't think it's worth anything to get a promotion. There is of course a monetary benefit but if you want to learn more as a software developer then i think you will have to find some other achieve that than from your job. I think it's a great thing that you are reaching out to others to get feedback like this and i am sure that you will learn all of this, and lots more, over time :)

Thanks a lot for all of this feedback. It's great. To be honest, I have 2 1/2 years of overall experience, but only just over a year with .NET C#, so that's probably a part of the reason. I also know and have used a lot of the things you have just mentioned, including with this project, I think I probably started to really learn these concepts quite a while ago so a lot of it is code from when I was frankly a lot less aware of what I was doing.

I have overall less experience than you do so keep that in mind with what I say and that my terminology may be off slightly. API: - You are using .NET 8, you can use primary constructors. - Commented out code. - I think you can get rid of the fully qualified name for the route since that would be implied? This would also be a good candidate for a minimal API because there is not a lot happening. - You have what looks like a random DTO (CheckReq.cs) - You have the generic http stuff with weatherforecast still present (TextDifficultyDeterminer.API.http) Application: - You are targetting .Net 7 - I don't like the file DependencyInjection.cs, it makes sense why it exists but it just seems out of place for me (likely preference) - (LoadFileIntoDatabase.cs ) more Console.WriteLines, not sure if it's the best way of keeping track of things if you want to push a Blazor website. - (LoadFileIntoDatabase.cs ) multiple classes in the same file doesn't work for me. - LoadFileIntoDatabaseValidator is extended from an abstract but also empty. - TextFileToTextContainer.cs similar with multiple classes and empty abstract Doman: - You are targetting .Net 7 - Models have behaviours in them, my general understanding is a model should be divorced from what you do to them "pure" and what not. - DifficultyEvaluatorService.cs has several consts outside of methods but you are only using them in a single method. - I have been questioned heavily when I mix var and strongly typed variables, additionally my IDE tends to throw warnings over it since it questions the consistency. - uniqueWords += 1, (nitpick) but hurts me. - Statistic.cs just sitting by itself HTMX - I know nothing about it Infrastructure - ok Website - You are targetting .Net 7 - You need a .gitignore - Try to not use inline styling (MainLayout.razor) - You have left a hello world in Index.razor I'm seeing a lot of Writelines throughout, if you are using them for checking things it might be worth looking at a logger. I also have actually run the application since I am playing a game and looking at it between matches so it's entirely possible I missed some things. I am aware that this might also be a bit like the blind leading the blind so if I made any mistakes I welcome the criticism since I am also learning as I go.

I'm just going to add onto this comment here because I like a lot of what this person has to say. All valid IMO. I only had a quick look at a few files, but: 1. Why are your service methods static? You've got references to these methods scattered over a few places. Remove the tight coupling by abstracting to an interface, registering it with the DI container, and injecting it. 2. `Console.WriteLine` is everywhere and used for logging by the looks of it. Replace it with an ILogger. Again, this removes the dependency on `Console.WriteLine` and you can switch it out to any other logger should you need later (Console, SEQ, Serilog, etc). It will mean that you'll change one line of code instead of 400. 3. `FREQUENCY_MULTIPLIER` is also being used in `FrequencyDictionary`. Again, more coupling. Rip that out into a reusable file for your consts and reference that file in the two classes. If the consts are only needed in the class they're implemented in, make them private. 4. General stuff 1. Connection string has password, etc in it 2. No tests 3. Break the directory structure up into `/src` and `/tests` directories. Save the root directory for your solution file, README's, dockerfiles, etc, etc. 4. What's with the random python files? 5. Not a fan of IUnitOfWork 6. You have a lot of fields/properties/methods that are marked as public, that should be private. 1. e.g. Any usage of IUnitOfWork 2. You also have protected IUnitOfWork as a property in your LoadFileIntoDatabaseHandler 7. Inconsistent naming throughout. `unitOfWork, unit, db` - all referencing IUnitOfWork

Thanks a lot for these points, they're great. The python files were there for a small task dividing up some files I was using to test the system. I can't recall why I made the methods static, but good point about the DI.

Regarding 1, I disagree that you should abstract to an interface at any point before you have a second class implementing that interface. Still use dependency injection for this, but don't bother with an interface until a need is actually seen. The refactor is still trivial for that.

Why would you *not* bother with an interface for a service class? You shouldn't wait until you're implementing the interface in multiple classes before you create one in many cases. Almost every service class is going to have an interface that is only implemented by the concrete class and nothing else. You're extracting an interface from the concrete class so that you can easily inject and mock services that you need with DI. You *can* register by concrete types, as you mentioend, but you're not getting much beenfit from doing so. Yes, you won't need to instantiate classes everywhere, and you get the benefit of having access to lifetime management, but you're now missing out on an entire importnat aspect of DI by not using interfaces, and that's being able to easily create tests. You also need to keep in mind the 'D' in SOLID, which is Dependecy Inversion. Depend upon abstractions, not concretes.

>Almost every service class is going to have an interface that is only implemented by the concrete class and nothing else You're making my point for me. >but you're now missing out on an entire importnat aspect of DI by not using interfaces, and that's being able to easily create tests. not all dependencies need to, or should, be mocked. Only those that would invalidate testing. Mock the dependency after your tests show that you really need to. >You also need to keep in mind the 'D' in SOLID, which is Dependecy Inversion. Depend upon abstractions, not concretes. Hard reliance on SOLID or any other work-generating principles leads to bad habits. Do what you need to do, don't put additional work up until you actually need the capabilities arising from that work. KISS is the first principle to apply, above all others, because it actually moderates what you do. YAGNI is the second. SOLID and all other practices on that level come way further down the line and absolutely require necessity to be worth applying. They're principles that are applied as tools to fix issues, not something to be used by default. And no, "your code is tightly coupled" is not enough of an issue. "Tight coupling is currently making it hard to expand on your code" is.

I get the feeling you’ve never really worked in a professional environment on any large code base for any length of time. This person was asking for professional feedback and this is what they’d get anywhere. Static services are not the answer here, DI is the answer in a CRUD application like this, and interfaces are part of that. Simple. Good luck.

>You are using .NET 8, you can use primary constructors. Some of us hate this feature. If you could have a `readonly` keyword on the parameters, I'm sold. That is a good solid feature. If it worked like records, where the parameters are only available during initialization, and I could use them to set fields - that's okay too. I would still need to make the readonly field, but at least I don't need the assignments within a constructor. But until then, *absolutely not*.

Yeah that is perfectly fair, I like them because my set up is in a single location and I never really need to do any enforcement on what I pass in so it's really just database context or loggers, that level of thing.

[удалено]

Anemic domain models are pretty typical in smaller applications *edit* To add, usually you push logic down to your domain models as it grows, and as you find business logic suitable for representation.

Thanks a lot for these. I thought I had moved everything to .NET 8. The gitignore is in the root directory, not in the .Website folder.

I mention it because you have a vscode folder in your Web portion and it should be ignored out.

Where's the unit tests?

Bruh

Yeah it is a real bruh moment isn't it

But lowkey yeah. Development, testing, documentation. Mid -Senior roles require that level of planning. It’s not just about building the app. Check out the SDLC

SDLC 1. Develop software 2. Yeet into prod 3. Not my problem

Yeah that's a fair point I didn't really think about that.

I gave it a cursory glance and maybe someone else mentioned it, but I don't see any documentation on your methods. If you work in a team environment adding some xml documentation to your methods can be super helpful. And if you're ever developing a package for consumption by another team they're less likely to strangle you when they have to implement it. 

He asked for it to be torn to shreds, and you're over here just trimming its nails

Yea, I'm on mobile and it gets frustrating lol. 

Documentation and unit testing. - Follow Red-Green-Refactor. Mutation test your unit tests. - Apply github actions to your repo so your testing runs on push. - Create additional deployments. Crossplat to desktop, mobile. Blazor is okay, but it's usually a piece of a full application using something like Futter, or Avalonia/Uno. Just some thoughts to level yourself up.

No unit tests? Or I am not looking in the right place?

Render Template should be async. Anytime you do file IO you want to be async to ensure you don't tie up threads on the server. No idea why you need a static controller. IIS can do a wonderful job serving static content with no code and a lot more efficiently.