project done - #222
Conversation
Dejmenek
left a comment
Dejmenek
left a comment
There was a problem hiding this comment.
Hey @mrqplayer 👋,
I've reviewed your project, and you've met all the project requirements. However, there are a few issues that need to be addressed before I can approve it. Most of them are related to validation and handling edge cases that can cause the application to crash. Taking care of these cases is important because it makes your application more robust. I've marked the blocking issues with 🔴
🟢 You've fulfilled all the project requirements🎉
⭐I noticed you didn't attempt the optional challenges, which is completely fine. However, I'd always encourage you to give them a try. They're a great opportunity to learn and push yourself a bit further💪.
Suggestions
I've also left a few comments with suggestions and ideas for improvement. They're non-blocking👍
🟡 I noticed that some classes are handling multiple responsibilities at once, including SQL operations, console input/output, and validation. Consider separating these parts to make the code easier to follow and maintain. This would also align with principles like Separation of Concerns, KISS, and DRY that were introduced in previous projects.
🟡 There are some redundant nested database connections. Currently, methods like DeleteFlashcards, UpdateFlashcards, and others open their own database connection and then call methods like ViewFlashcards or ViewStacks, which open another connection. This creates unnecessary overhead, and the operations could be handled using a single open connection. SQL Server has a limited connection pool, so it's good practice to use connections efficiently and avoid opening more than needed. This might not be a big issue for a small project, but in a real application with many users, inefficient connection handling could become a problem.
🟡 There are some naming and style inconsistencies. Flashcard and FlashcardDto use public mutable fields instead of properties. Additionally, the fields in FlashcardDto (question, answer) use camelCase, which doesn't follow the standard C# naming convention where public members should use PascalCase.
🟡 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 it comes with a lot of useful features that make building console applications much easier.
🟡 I noticed there are empty reader.cs and test.cs classes included in the project. Since they are not used anywhere, they add unnecessary code and could be removed to keep the project cleaner.
|
|
||
| var endDateTime = DateTime.Now; | ||
|
|
||
| SaveSession(stackId, goodAnswer, totalQuestions, startDateTime, endDateTime); |
There was a problem hiding this comment.
🔴 When there are no stacks in the database and I try to select a stack to study, this exception is thrown because the application attempts to save a study session using a stackId that doesn't exist.
| while (isRunning) | ||
| { | ||
| Console.WriteLine("Select option from menu below"); | ||
| Console.WriteLine("----------------------------------"); | ||
| Console.WriteLine("0. Exit\n"); | ||
| Console.WriteLine("1. View stacks\n"); | ||
| Console.WriteLine("2. Add stack\n"); | ||
| Console.WriteLine("3. Delete stack\n"); | ||
| Console.WriteLine("4. Update stack\n"); | ||
| Console.WriteLine("5. Edit flashcard\n"); | ||
| Console.WriteLine("6. Manage flashcards\n"); | ||
| Console.WriteLine("7. Add flashcards\n"); | ||
| Console.WriteLine("8. View flashcards\n"); | ||
| Console.WriteLine("9. Delete flashcards\n"); | ||
| Console.WriteLine("10. Update flashcards\n"); | ||
| Console.WriteLine("11. Start Session\n"); | ||
| Console.WriteLine("12. View Sessions\n"); | ||
|
|
||
| Console.WriteLine("----------------------------------"); | ||
|
|
||
| string? input = Console.ReadLine(); | ||
|
|
||
| int choice; | ||
|
|
||
| bool success = int.TryParse(input, out choice); | ||
| while (!success) | ||
| { | ||
| Console.WriteLine("Please enter a number, to select menu option"); | ||
| input = Console.ReadLine(); | ||
| success = int.TryParse(input, out choice); | ||
| } | ||
| switch (choice) | ||
| { | ||
| case 0: | ||
| Console.Clear(); | ||
| Console.Write("\nGoodbye\n"); | ||
| Environment.Exit(0); | ||
| break; | ||
|
|
||
| case 1: | ||
| Console.Clear(); | ||
| Stacks.ViewStack(); | ||
| break; | ||
|
|
||
| case 2: | ||
| Console.Clear(); | ||
| Stacks.NewStack(); | ||
| break; | ||
|
|
||
| case 3: | ||
| Console.Clear(); | ||
| Stacks.DeleteStack(); | ||
| break; | ||
|
|
||
| case 4: | ||
| Console.Clear(); | ||
| Stacks.UpdateStack(); | ||
| break; | ||
|
|
||
| case 5: | ||
| Console.Clear(); | ||
| Stacks.ViewStack(); | ||
| break; | ||
|
|
||
| case 6: | ||
| Stacks.ViewStack(); | ||
| Console.Clear(); | ||
| Console.WriteLine(""); | ||
| break; |
There was a problem hiding this comment.
🟡 Menu items 5 and 6 don't seem to work as intended. Edit flashcard calls Stacks.ViewStack() instead of any flashcard editing logic, so it appears to be duplicate or leftover code since menu item 10 already provides the editing functionality
| public static void NewStack() | ||
| { | ||
| using (var connection = Database.GetConnection()) | ||
| { | ||
| Console.WriteLine("Enter stack name"); | ||
| string name = Console.ReadLine(); | ||
| while (string.IsNullOrWhiteSpace(name)) | ||
| { | ||
| Console.WriteLine("Enter stack name"); | ||
| name = Console.ReadLine(); | ||
| } | ||
|
|
||
| var tableCmd = connection.CreateCommand(); | ||
|
|
||
| tableCmd.CommandText = | ||
| @"INSERT INTO stacks | ||
| (name) VALUES (@name) ;"; | ||
|
|
||
| tableCmd.Parameters.AddWithValue("@name", name); | ||
|
|
||
| tableCmd.ExecuteNonQuery(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 When the database throws a SqlException because of a UNIQUE constraint violation on the stack name, the exception isn't caught anywhere, causing the application to crash. Since this is a recoverable scenario, it would be better to catch the exception and display a user-friendly message instead. The same happens when trying to update a stack's name.
| public static void AddFlashcards() | ||
| { | ||
| using (var connection = Database.GetConnection()) | ||
| { | ||
| Console.WriteLine("Please insert stack ID for new flashcard"); | ||
| Stacks.ViewStack(); | ||
|
|
||
| string idInput = Console.ReadLine(); | ||
| int input; | ||
| bool success = int.TryParse(idInput, out input); | ||
| while (!success) | ||
| { | ||
| Console.WriteLine("Please insert stack ID for new flashcard"); | ||
| idInput = Console.ReadLine(); | ||
| success = int.TryParse(idInput, out input); | ||
| } | ||
|
|
||
| Console.WriteLine("Please insert question for new flashcard"); | ||
| string question = Console.ReadLine(); | ||
| while (string.IsNullOrWhiteSpace(question)) | ||
| { | ||
| Console.WriteLine("Please insert question for new flashcard"); | ||
| question = Console.ReadLine(); | ||
| } | ||
|
|
||
| Console.WriteLine("Please insert answer for new flashcard"); | ||
| string answer = Console.ReadLine(); | ||
| while (string.IsNullOrWhiteSpace(answer)) | ||
| { | ||
| Console.WriteLine("Please insert answer for new flashcard"); | ||
| answer = Console.ReadLine(); | ||
| } | ||
|
|
||
| var tableCmd = connection.CreateCommand(); | ||
|
|
||
| tableCmd.CommandText = | ||
| @"INSERT INTO flashcards (stackId, question, answer) | ||
| VALUES (@stackId, @question, @answer);"; | ||
|
|
||
| tableCmd.Parameters.AddWithValue("@stackId", input); | ||
| tableCmd.Parameters.AddWithValue("@answer", answer); | ||
| tableCmd.Parameters.AddWithValue("@question", question); | ||
|
|
||
| tableCmd.ExecuteNonQuery(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 There's no validation that the entered stack ID exists before inserting a new flashcard, which causes the application to crash when an invalid ID is provided.
| { | ||
| public class Database | ||
| { | ||
| public static string connectionString = @"Server = KOMPUTER-K;Database = FlashcardsDb;TrustServerCertificate=True;Integrated Security=True"; |
There was a problem hiding this comment.
🟡 It's a good practise to move the connection string to for example appsettings.json. It's easier to maintain and update that way.
2 participants
No description provided.