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 #93

Closed
wants to merge 6 commits into from
Closed

Assignment4 #93

wants to merge 6 commits into from

Conversation

@SScura
Copy link

SScura commented Feb 6, 2020

No description provided.

Copy link
Collaborator

Keboo left a comment

(SScura)

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 ✔
  • Controllers are all unit tested - Lots of controller methods untested.

Extra Credit

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

Peer Review

  • On time
  • Accurate
  • Appropriate comments given
private class UserService : IUserService
{
private Dictionary<int, User> Items { get; } = new Dictionary<int, User>();
public Task<bool> DeleteAsync(int id)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Parameter id of method DeleteAsync is never used. Remove the parameter or use it in the method body.

return Task.FromResult(Items[id]);
}

public Task<User?> UpdateAsync(int id, User entity)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Parameter id of method UpdateAsync is never used. Remove the parameter or use it in the method body.
Parameter entity of method UpdateAsync is never used. Remove the parameter or use it in the method body.

@@ -92,10 +72,10 @@ public async Task FetchByIdAsync_WithExitingItem_RemovesIt()

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Local variable 'service' should be used

[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<ActionResult> Delete(int id)
{
if (await EntityService.DeleteAsync(id) is true)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator
Suggested change
if (await EntityService.DeleteAsync(id) is true)
if (await EntityService.DeleteAsync(id))
@@ -1,11 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<Nullable Condition="$(Nullable)==''">enable</Nullable>
<TargetFramework>netcoreapp3.1</TargetFramework>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Why remove the nullable property?

{
public class Startup
{
[System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "<Pending>")]

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Is this needed? The method appears to be static anyway

public class Startup
{
[System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "<Pending>")]
[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "<Pending>")]

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

A #pragma might have been easier here.

services.AddSwaggerDocument();
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2007:Consider calling ConfigureAwait on the awaited task", Justification = "<Pending>")]

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

I am not sure which line this is referring to.

@@ -15,26 +15,6 @@ public abstract class EntityServiceTests<TEntity> : TestBase where TEntity : Ent

protected abstract TEntity CreateEntity();

[TestMethod]

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Is there a reason this test was removed. These tests should not have required much modification.


// Assert
Assert.AreEqual(entity.Id, found.Id);
//Assert.AreEqual(entity.Id, found.Id);

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

I would remove the test or mark it as ignored before I would remove the assets. This gives the false sense that the test is passing when in fact it is not doing anything.

@Keboo Keboo closed this Feb 11, 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

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