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

Assignment4 #83

Closed
wants to merge 13 commits into from
Closed

Conversation

@gaberies
Copy link

gaberies commented Feb 4, 2020

Collaborated with @HMRich

We did our own peer review

  • WebAPI Analyzers enabled ✔
  • DI properly configured
    • Services properly registered with correct scopes ✔
    • DB context properly registered ✔
    • AutoMapper properly registered ✔
  • NSwag properly setup ✔
  • API controllers properly expose CRUD operations ✔
  • Controllers are all unit tested ✔
HMRich and others added 12 commits Feb 3, 2020
changed from             Task<IEnumerable<User>> returnedValue = (Task<IEnumerable<User>>)await controller.Get(); to List<User> returnedValue = (List<User>)await controller.Get();
@gaberies gaberies marked this pull request as ready for review Feb 4, 2020
_ = new GiftController(service);

//Assert
Assert.IsTrue(service != null);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 7, 2020

Collaborator

In this case you are simply asserting that your constructor did not throw an exception so you don't need this assert

ActionResult<Gift> rv = await controller.Get(gift.Id);

// Assert
Assert.IsTrue(rv.Result is OkObjectResult);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 7, 2020

Collaborator

OkObjectResult has a value property on it. Given that the test is to verify that you can retrieve an item by id, it would also be good to check that the value returned was a Gift with a matching id.

Copy link
Collaborator

Keboo left a comment

(gaberies) (HMRich)

Assignment4

Project Analysis Summary
Nullable Enabled:
Unexpected Disabled Warnings:
Appropriately handle all warnings:
ProjectName Nullable Solutions Analyzers Unexpected Disabled Warnings Waning Level
SecretSanta.Api Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.CSharp.Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4
SecretSanta.Api.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.CSharp.Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4

Main Assignment

  • WebAPI Analyzers enabled
  • DI properly configured ✔
  • Services properly registered with correct scopes ✔
  • DB context properly registered ✔
  • AutoMapper properly registered ✔
  • NSwag properly setup ✔
  • API controllers properly expose CRUD operations - Crud operations are there, but there are some issues with the implementation.
  • Controllers are all unit tested ✔

Extra Credit

  • Unit tests in EntityServiceTests fixed appropriately
  • Mocking framework used to create the test doubles ✔

Peer Review

  • On time ✔
  • Accurate ✔
  • Appropriate comments given ✔

}

internal class MockUserService : IUserService

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Internal type is never instantiated. Consider using it, removing it, or making it static (if it contains static members).


}

internal class MockGroupService : IGroupService

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Internal type is never instantiated. Consider using it, removing it, or making it static (if it contains static members).


}

internal class MockGiftService : IGiftService

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Internal type is never instantiated. Consider using it, removing it, or making it static (if it contains static members).

[ProducesResponseType(StatusCodes.Status400BadRequest)]
public async Task<ActionResult<Gift>> Post(Gift? value)
{
if (value != null)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

This check is handled for you by ASP.NET Core. Outside of a unit test that directly calls this method, this case is not possible.

[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<ActionResult<Group>> Get(int id)
{
if (await GroupService.FetchByIdAsync(id) is Group group && group != null)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

The null check here is not needed. The pattern matching ensures that the value is not null.

Suggested change
if (await GroupService.FetchByIdAsync(id) is Group group && group != null)
if (await GroupService.FetchByIdAsync(id) is Group group)
[ProducesDefaultResponseType]
public async Task<ActionResult<Group>> Put(int id, Group value)
{
if (value != null && GroupService.FetchByIdAsync(id) is Task<Group> groupResult)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

FetchByIdAsync is an async method. You should be awaiting this method, not acting on the Task directly.

Suggested change
if (value != null && GroupService.FetchByIdAsync(id) is Task<Group> groupResult)
if (await GroupService.FetchByIdAsync(id) is Group groupResult)
{
return Ok(group);
}
return StatusCode(500);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

You never want to return a 500 status code. The 500-599 status code range means there is an error with your service (typically your service threw an exception). NotFound would be more appropriate here.

[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<ActionResult<Group>> Delete(int id)
{
if (GroupService.FetchByIdAsync(id) is Task<Group> group && group != null)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 10, 2020

Collaborator

Because this call is not awaited, this check will always pass.

@Keboo Keboo closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.