Push code for review - #219
Conversation
|
@Dejmenek I've added functions for database initialization. Could you please review it? Thank you. |
Dejmenek
left a comment
Dejmenek
left a comment
There was a problem hiding this comment.
Thank you for making the changes👌
I've reviewed your project, and it looks good. However, there's one requirement that is still missing, which prevents me from approving the project.😔
🔴 Stack names should be unique. Currently, it's possible to create multiple stacks with the same name, which shouldn't be allowed
⭐ I really like that you challenged yourself and tried to complete the additional challenge. Your solution already handles part of the requirement by displaying the number of sessions per month. The only missing piece is grouping the results by stack, so each stack has its own monthly session count. Here's how it should look like:
Suggestions
I've left some comments with suggestions and ideas for improvement👍
🟡 I highly recommend moving the connection string to appsettings.json and loading it through ConfigurationBuilder/IConfiguration. This keeps configuration separate from the code and makes future changes easier, as you only need to update the value in one place instead of searching through the codebase.
🟡 I was wondering why you decided not to use Spectre.Console for this project. I think it could simplify quite a bit of the code and provide a lot of useful features that make building console applications easier. It was also used in the previous project.
Please fix the 🔴 issue, as it blocks the approval of your project. Also, feel free to reach out if you have any questions😁
| if(inp != null && (inp.Trim() == "A" || inp.Trim() == "a")) | ||
| { |
There was a problem hiding this comment.
🟡 After providing an invalid number and then trying to create a new stack by typing A, it's no longer possible because the input is still being treated as a number. I have to exit stack management and enter it again before I can create a new stack. The same thing applies to the FlashcardMenu file as well.
|
|
||
| if (isSuccess) | ||
| { | ||
| Console.WriteLine("\nCreated new stack successfully!"); |
There was a problem hiding this comment.
🟡 This message should say something like "Created new flashcard successfully" instead of referring to a stack.
| using FlashcardsApp.Models; | ||
| using FlashcardsApp.DTOs; | ||
|
|
||
| namespace FlashcardsApp.Repositories |
There was a problem hiding this comment.
🟡 To reduce one level of indentation, you could use file-scoped namespaces
| var newFlashcard = new Flashcard { Id = id, StackId = stackId, Front = front, Back = back }; | ||
|
|
||
| try |
There was a problem hiding this comment.
🟡 There's no need to add blank lines between try, catch, and finally blocks.
| } | ||
| } | ||
|
|
||
| public void showStackFlashcard(FStack fstack) |
There was a problem hiding this comment.
🟡 I noticed that the naming convention for methods is not consistent. In C#, the standard convention for method names is PascalCase, but in this example, camelCase was used instead. I highly recommend checking out this article about C# code conventions: https://thecsharpacademy.com/article/58/code-conventions
2 participants
No description provided.