Skip to content
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

Console.Flashcards - Finished #137

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

FunRunRushFlush
Copy link

Hello C# Academy,

This is my solution to the Console Flashcards challenge.
I would greatly appreciate your feedback on my implementation

@FunRunRushFlush
Copy link
Author

ReadME Bug (Codacy Static Code Analysis)

@chrisjamiecarter chrisjamiecarter self-assigned this Feb 24, 2025
Copy link
Collaborator

@chrisjamiecarter chrisjamiecarter left a comment

Choose a reason for hiding this comment

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

Hey @FunRunRushFlush 👋,

Excellent work on your Flashcards project submission 🎉 !

I have performed a peer review. Review/ignore any comments as you wish.


🟢 Requirements

⭐ You have fulfilled all of the project requirements!

__

🟢 Lovely UI

__

🟠 Project Submission Structure

💡 Looks like you named your project : Flashcards.FunRunRushFlush. But what we need is an outer "project submission" folder called this. This should contain all your files. i.e. your solution, images and readme are all outside your submission folder.

👨‍🏫 For example,

Create your project submission folder in the forked root:
image

Then add your solution and project inside your project submission folder:
image

💭 This will make it easier for code-reviewers to pull down, find and review your code in the future. This will also make sense when you get to the projects with other students code in and it looks like this:
image

__

🟠 Style Guide Compliance

💡 Remember that when you have finished a project, give it a polish pass. Refer to the Academy's code-conventions page.

💭 This is important as good code conventions/styling ensures consistency, makes your code easier for others to read and maintain, and improves your chances of getting a pull request reviewed and approved.

__

🟠 Error Handling UX

💡 Tried to add a duplicate stack, it errored 👍 !

💭 Don't show the exception to the user 👎 - even though it is only for a split second, show a sanitised message instead, i.e. Stack with name 'blah' already exists. Or. Unable to add duplicate stack.


I will go ahead and mark as approved, keep up the excellent work on the next projects! 😊

Best regards,
@chrisjamiecarter 👍





Copy link
Collaborator

Choose a reason for hiding this comment

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

🟠 White-space

💡 All this random extra white-space is redundant and looks messy. Removing all this white-space is a quick win to get your code looking more professional.

@@ -0,0 +1,10 @@
using Flashcards.FunRunRushFlush.Data.Model;

namespace Flashcards.FunRunRushFlush.App.Interfaces
Copy link
Collaborator

Choose a reason for hiding this comment

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

🟠 Code Consistency

💡 You use file-scoped namespaces elsewhere, but block-scoped namespaces here. Be consistent in your choices. It is better to be consistent that right. But if you want to also be right, C# academy guidelines that you should be using file-scoped namespaces everywhere.

using Microsoft.Extensions.Logging;
using System.Data;

var host = Host.CreateDefaultBuilder(args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

🟢 Dependency Injection

Copy link
Collaborator

Choose a reason for hiding this comment

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

🟢 README File

Copy link
Collaborator

Choose a reason for hiding this comment

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

🟢 Configuration

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.

3 participants