refactor: consolidate player data into single source of truth (#210)#352
Conversation
Eliminate duplication between PlayerData.cs and PlayerFakes.cs by making PlayerFakes delegate to PlayerData methods. - PlayerFakes.MakeStarting11() now calls PlayerData.MakeStarting11() - PlayerFakes.MakeNew() now uses PlayerData.GetSubstitutes() - All factory methods updated to delegate to PlayerData - Enhanced XML documentation to clarify SSOT relationship
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
WalkthroughRemoved feature/* from CI triggers and added comments in the pipeline; test utilities were updated to consume centralized PlayerData (starting XI and substitutes), assign new GUIDs to generated Players, and add in-memory SQLite helpers plus several mock/utility methods and XML docs. Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@azure-pipelines.yml`:
- Around line 6-14: The pipeline trigger list was broadened to include many
branch patterns (refactor/*, fix/*, feat/*, chore/*, docs/*, test/*) which
conflicts with the repo policy—restore the triggers to only "master" and
"feature/*" and ensure PR triggers still target master; update the trigger block
in azure-pipelines.yml (the trigger: section) to remove the extra patterns and
leave only the allowed entries so CI runs match policy.
Remove feature/* from pipeline triggers to avoid running CI on every push to development branches. CI now only runs on: - Pushes to master (production) - Pull requests to master (validation before merge) Development branches using conventional commit naming (refactor/*, fix/*, feat/*, etc.) will only trigger CI when a PR is opened, not on every commit. This maintains code quality validation while reducing Azure DevOps pipeline execution costs.
nanotaboada
force-pushed
the
refactor/issue-210-consolidate-player-data-ssot
branch
from
a20854a to
ac62c9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@azure-pipelines.yml`:
- Around line 5-11: The pipeline trigger was narrowed to only "master" but
should also include feature branches; update the trigger block (the trigger YAML
that currently lists "master") to add "feature/*" so the pipeline runs on
commits to master and feature/* branches per the coding guidelines, or if the
policy changed update the guideline text instead to reflect the new behavior.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Dotnet.Samples.AspNetCore.WebApi/Utilities/PlayerData.cs (1)
43-44: Typo: "Altético Madrid" should be "Atlético Madrid".The team name is misspelled as "Altético" (missing the accent on the 'l') in multiple player entries. Note that the JSON data in
MakeStarting11FromDeserializedJson()correctly uses "Atlético Madrid" (lines 779, 839).📝 Proposed fix
- Team = "Altético Madrid", + Team = "Atlético Madrid",Apply this fix at lines 43, 108, 196, and 266.
Also applies to: 108-109, 196-197, 266-267
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/DatabaseFakes.cs (1)
51-62: Incomplete table schema and potential inconsistency with EF Core model.The
CreateTable()method contains placeholder SQL (/* ... other columns ... */) that will create an incomplete schema missing most columns. Additionally, this raw SQL approach may drift from the actual entity configuration inPlayerDbContext.OnModelCreating().Consider using EF Core's built-in schema creation instead:
🔧 Recommended fix using EF Core schema management
public static void CreateTable(this PlayerDbContext context) { - using var cmd = context.Database.GetDbConnection().CreateCommand(); - cmd.CommandText = - @" - CREATE TABLE players ( - id INTEGER PRIMARY KEY, - firstName TEXT NOT NULL, - /* ... other columns ... */ - )"; - cmd.ExecuteNonQuery(); + context.Database.EnsureCreated(); }
EnsureCreated()generates the schema from the EF Core model, ensuring consistency withPlayerDbContext.OnModelCreating()configuration (including the unique index onSquadNumber).test/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerMocks.cs (2)
46-52: Incorrect use ofIt.IsAny<string>()as return value.
It.IsAny<T>()is a Moq argument matcher for setup expressions, not a value producer. Using it in.Returns()will returndefault(string)(null), not "any string". If the intention is to return a non-null placeholder URL:🔧 Proposed fix
public static Mock<IUrlHelper> SetupUrlHelperMock() { var mock = new Mock<IUrlHelper>(); - mock.Setup(url => url.Action(It.IsAny<UrlActionContext>())).Returns(It.IsAny<string>()); + mock.Setup(url => url.Action(It.IsAny<UrlActionContext>())).Returns("http://localhost/api/players"); return mock; }Alternatively, if tests need dynamic URLs based on input, use a callback with the
UrlActionContextparameter.
66-75: Incorrect use ofIt.IsAny<object>()as a value.On line 70, when
cacheValueis null,It.IsAny<object>()is passed toSetupMemoryCacheMock(). This is incorrect becauseIt.IsAny<T>()is an argument matcher, not a value—it returnsdefault(object)(null) in this context. If the intent is to simulate an empty/null cache value, just passnulldirectly.📝 Proposed fix
- var memoryCache = SetupMemoryCacheMock(cacheValue ?? It.IsAny<object>()); + var memoryCache = SetupMemoryCacheMock(cacheValue);The
SetupMemoryCacheMockalready handles null values correctly by returningnullfromTryGetValuewhen appropriate.
🤖 Fix all issues with AI agents
In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs`:
- Around line 40-44: Replace incorrect ArgumentNullException thrown when a
lookup by squadNumber fails with a semantically appropriate exception (e.g.,
InvalidOperationException or ArgumentException) for consistency; specifically
update the expression using PlayerData.MakeStarting11().SingleOrDefault(p =>
p.SquadNumber == squadNumber) ?? throw new ArgumentNullException(...) to throw
InvalidOperationException (or ArgumentException) including the same descriptive
message, and apply the same replacement to the other similar lookups in
PlayerFakes (the other two occurrences that currently throw
ArgumentNullException).
🧹 Nitpick comments (2)
src/Dotnet.Samples.AspNetCore.WebApi/Utilities/PlayerData.cs (1)
165-325: Consider extracting shared player definitions to reduce duplication.
MakeStarting11WithId()andGetSubstitutesWithId()duplicate all player data from their non-ID counterparts, only adding theIdproperty. This creates a maintenance burden where any player data change must be applied in two places.Consider a helper approach that assigns IDs to an existing list:
public static List<Player> MakeStarting11WithId() { var players = MakeStarting11(); var guids = new[] { /* predefined GUIDs */ }; for (int i = 0; i < players.Count; i++) players[i].Id = guids[i]; return players; }This would ensure data consistency while preserving the deterministic GUIDs needed for testing.
Also applies to: 530-746
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs (1)
173-192: Side effect in LINQ Select: mutating source objects.The
Selectlambda assignsplayer.Id = Guid.NewGuid()which mutates thePlayerobjects returned byPlayerData.MakeStarting11(). While this works becauseMakeStarting11()creates new instances on each call, it's unexpected behavior in a projection and could cause subtle bugs if the implementation changes.Consider creating new instances or assigning the ID before the projection:
♻️ Cleaner approach
public static List<PlayerResponseModel> MakeResponseModelsForRetrieve() => - [ - .. PlayerData - .MakeStarting11() - .Select(player => - { - player.Id = Guid.NewGuid(); - return new PlayerResponseModel + PlayerData + .MakeStarting11() + .Select(player => new PlayerResponseModel + { + FullName = + $"{player.FirstName} {(string.IsNullOrWhiteSpace(player.MiddleName) ? "" : player.MiddleName + " ")}{player.LastName}".Trim(), + Birth = $"{player.DateOfBirth:MMMM d, yyyy}", + Dorsal = player.SquadNumber, + Position = player.Position, + Club = player.Team, + League = player.League, + Starting11 = player.Starting11 ? "Yes" : "No" + }) + .ToList();The
Idassignment appears unused forPlayerResponseModel(which doesn't have anIdproperty based on the relevant snippets), so it can be removed entirely.
| var player = | ||
| MakeStarting11().SingleOrDefault(player => player.SquadNumber == squadNumber) | ||
| PlayerData.MakeStarting11().SingleOrDefault(p => p.SquadNumber == squadNumber) | ||
| ?? throw new ArgumentNullException( | ||
| $"Player with Squad Number {squadNumber} not found." | ||
| ); |
There was a problem hiding this comment.
Inconsistent exception types for "not found" scenarios.
Line 42 throws ArgumentNullException for a lookup failure, while line 59 throws InvalidOperationException for a similar case. ArgumentNullException is semantically incorrect here since the argument isn't null—the lookup simply returned no result. Consider using InvalidOperationException or ArgumentException consistently.
📝 Proposed fix for consistency
var player =
PlayerData.MakeStarting11().SingleOrDefault(p => p.SquadNumber == squadNumber)
- ?? throw new ArgumentNullException(
+ ?? throw new InvalidOperationException(
$"Player with Squad Number {squadNumber} not found."
);Apply similar changes at lines 126-128 and 152-154.
Also applies to: 56-61
🤖 Prompt for AI Agents
In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/PlayerFakes.cs` around
lines 40 - 44, Replace incorrect ArgumentNullException thrown when a lookup by
squadNumber fails with a semantically appropriate exception (e.g.,
InvalidOperationException or ArgumentException) for consistency; specifically
update the expression using PlayerData.MakeStarting11().SingleOrDefault(p =>
p.SquadNumber == squadNumber) ?? throw new ArgumentNullException(...) to throw
InvalidOperationException (or ArgumentException) including the same descriptive
message, and apply the same replacement to the other similar lookups in
PlayerFakes (the other two occurrences that currently throw
ArgumentNullException).
1 participant
Eliminate duplication between PlayerData.cs and PlayerFakes.cs by
making PlayerFakes delegate to PlayerData methods.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.