Skip to content

Yuhua's PR #3

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Yuhua's PR #3

wants to merge 1 commit into from

Conversation

celianiu
Copy link

@celianiu celianiu commented Nov 7, 2023

Sorry for single commit message; Forget to fork to my own repo

@celianiu
Copy link
Author

celianiu commented Nov 8, 2023

image

private readonly IArticleRepository articleRepository = null;
private readonly IUserRepository userRepository = null;

public ArticleService(ArticleStore articleStore, UserStore userStore, IArticleRepository articleRepository, IUserRepository userRepository)

Choose a reason for hiding this comment

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

Good job: dependency injection

Article article = new Article(userNameWhoWillAdd, articleTitle, articleContent);
User newUser = new User(userNameWhoWillAdd);

Choose a reason for hiding this comment

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

Good mock.

@jingwang315
Copy link

Good points:

  1. Use DI container to register all single and scoped dependencies.
  2. Apart from UserController test, all required test passed.
  3. 3 layers of controller.
  4. Commit step by step.
  5. Readable commit message

{
//this.articleStore = articleStore;

Choose a reason for hiding this comment

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

suggestion: remove dead code

{
return await articleService.GetAll();
Console.WriteLine(articleService.GetAllAsync());

Choose a reason for hiding this comment

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

suggestion: remove code for debug

@@ -13,59 +15,45 @@ public class UserController : ControllerBase
{
private readonly ArticleStore articleStore = null!;
private readonly UserStore userStore = null!;
private readonly ArticleService articleService = null!;

Choose a reason for hiding this comment

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

suggestion: remove unused articleService


namespace MiniBlog.Repositories
{
public class UserRepository : IUserRepository

Choose a reason for hiding this comment

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

well: nice to also provide repository for user

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