-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Project finish #139
base: main
Are you sure you want to change the base?
Project finish #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nikosnick13 👋,
Excellent work on your Flashcards project submission 🎉 !
I have performed a peer review. Review/ignore any comments as you wish.
🔴 App Does Not Build
🐞 Lots of errors in StudySessionController
that need resolving.
💡 Make sure you run and test your app before submission!
Please revisit to try understand and fix any issues marked 🔴 , as they block my approval 🚫 .
Feel free to reach out if you have any questions or if you'd like to collaborate further on any of these points 🆘 .
I will go ahead and wait for you to make your changes, commit and push (which will then notify me) 😊 !
Thanks,
@chrisjamiecarter 👍
private string? connectionString = ConfigurationManager.AppSettings.Get("ConnectionString"); | ||
|
||
|
||
public void InsertFlashcart(BasicFlashcardDTO flashcards) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Typo
😆 Easily done!
❌ InsertFlashcart
✔️ InsertFlashcard
{ | ||
try | ||
{ | ||
using var conn = new SqlConnection(connectionString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Using
⭐ Using using
statements on Connections.
|
||
} | ||
|
||
//TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Correct comment?
💡 Is this a true statement? Is this TODO? If so, try make a rule that you don't deliver any thing with a TODO comment. Tech debt, you wont come back to it, it will live as a todo forever. If not, try to make sure you do a final revisit and clean up any code comments.
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
namespace Flashcards.nikosnick13.DTOs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Mixed Conventions
💡 You have used block-scoped namespaces here, but file-scoped everywhere else. Try to stick to your conventions, it is better to be consistent than correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 README File
⭐ Great README, well done!
return userInputAnswer; | ||
} | ||
|
||
//NOT WORKING TODO: FIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Comment
💡 Same as before
@@ -0,0 +1,17 @@ | |||
namespace FlashStudy.Models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Incorrect Namespace
❗ Currently your app does not compile/build.
💡 I had to change to Flashcards.nikosnick13.Models
to get it to work.
I finish the project. I am waiting for the review :)