Skip to content

Finished guided project habit tracker#5

Open
Hacker-735 wants to merge 1 commit into
the-csharp-academy:masterfrom
Hacker-735:master
Open

Finished guided project habit tracker#5
Hacker-735 wants to merge 1 commit into
the-csharp-academy:masterfrom
Hacker-735:master

Conversation

@Hacker-735

Copy link
Copy Markdown

this is my first CRUD application which is exciting

@DSills735 DSills735 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello ✋ @Hacker-735 ,

I am here to perform your peer review, I apoligize for the delay. Feel free to work on the future roadmap projects while you are waiting on our review.

Anything with the 🔴 needs to be fixed and blocks approval. 🟠 Is not a required fix, but is my reccomendation for cleaner code. 🟢 Are things I found that were done particularly well.

🔴 Blocking approval

  • Nothing! Good work.

🟠 Needs work
-UI is messy in some areas, but cleaner in others. Use Console.Clear() as much as possible. This really will clean up a UI in a console app.

  • I cannot type td if I mis input the date originally. Not a big deal, but definitely an annoyance if In am working with this app regularly.

  • I cannot dissaprove now because of this, but with SQL commands, it is standard to not use string interpolation. Interpolation in commands is where SQL injection comes from, and is a major security risk.

-This is not a requirement of this project, but if you get much bigger than this,code becomes hugely disorganized. Using good SOC practice early and often will help alot. (I believe this is a requirement for all projects moving forward.)

🟢 Good things

-UI is clean with options 3/4.

-Validations are good and accrately handle edge cases.

  • Other than the SQL issues, code is written cleanly, I can easily understand the intentions of code. Once note I will make is consider CS8600. This is "just a warning", but it does have its place. You do properly handle it. How can we make this warning dissapear? No need to fix this, just tryiong to provoke thought for the future.

Thank you! Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants