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

v3.6.0 #61

Merged
merged 14 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Nhl.Api.Common/Http/NhlScoresHtmlReportsApiHttpClient.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace Nhl.Api.Common.Http;
/// <summary>
Expand All @@ -8,7 +10,7 @@
public class NhlScoresHtmlReportsApiHttpClient : NhlApiHttpClient
{
private static readonly object _lock = new object();
private static HttpClient _httpClient;

Check warning on line 13 in Nhl.Api.Common/Http/NhlScoresHtmlReportsApiHttpClient.cs

View workflow job for this annotation

GitHub Actions / build

Non-nullable field '_httpClient' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.

/// <summary>
/// The NHL endpoint for HTML reports
Expand Down
9 changes: 7 additions & 2 deletions Nhl.Api.Domain/Models/Game/GameCenterPlayByPlay.cs
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,7 @@ public class GameCenterPlay
/// The period number for the NHL game center play by play <br/>
/// Example: 1
/// </summary>
[JsonProperty("period")]
public int Period { get; set; }
public int Period => this.PeriodDescriptor?.Number ?? 0;

/// <summary>
/// The period descriptor for the NHL game center play by play <br/>
Expand Down Expand Up @@ -364,6 +363,12 @@ public class GameCenterPlay
/// </summary>
[JsonProperty("details")]
public GameCenterDetails Details { get; set; }

/// <summary>
/// The estimated time of the play for the NHL game center play by play, these are not exact times and are close approximations of each game event <br/>
/// Example: 2024-01-13T20:12:23Z
/// </summary>
public DateTimeOffset EstimatedDateTimeOfPlay { get; set; }
}

/// <summary>
Expand Down
31 changes: 30 additions & 1 deletion Nhl.Api.Domain/Models/Season/SeasonYears.cs
Original file line number Diff line number Diff line change
@@ -1,11 +1,40 @@


using System.Collections.Generic;

namespace Nhl.Api.Models.Season;

/// <summary>
/// All of the NHL season years since the inception of the NHL league
/// </summary>
public class SeasonYear
{
/// <summary>
/// A collection of all the NHL seasons
/// </summary>
public static readonly HashSet<string> AllSeasons =
[
"19171918", "19181919", "19191920", "19201921", "19211922", "19221923",
"19231924", "19241925", "19251926", "19261927", "19271928", "19281929",
"19291930", "19301931", "19311932", "19321933", "19331934", "19341935",
"19351936", "19361937", "19371938", "19381939", "19391940", "19401941",
"19411942", "19421943", "19431944", "19441945", "19451946", "19461947",
"19471948", "19481949", "19491950", "19501951", "19511952", "19521953",
"19531954", "19541955", "19551956", "19561957", "19571958", "19581959",
"19591960", "19601961", "19611962", "19621963", "19631964", "19641965",
"19651966", "19661967", "19671968", "19681969", "19691970", "19701971",
"19711972", "19721973", "19731974", "19741975", "19751976", "19761977",
"19771978", "19781979", "19791980", "19801981", "19811982", "19821983",
"19831984", "19841985", "19851986", "19861987", "19871988", "19881989",
"19891990", "19901991", "19911992", "19921993", "19931994", "19941995",
"19951996", "19961997", "19971998", "19981999", "19992000", "20002001",
"20012002", "20022003", "20032004", "20052006", "20062007", "20072008",
"20082009", "20092010", "20102011", "20112012", "20122013", "20132014",
"20142015", "20152016", "20162017", "20172018", "20182019", "20192020",
"20202021", "20212022", "20222023", "20232024", "20242025", "20252026",
"20262027", "20272028", "20282029", "20292030", "20302031", "20312032",
"20322033", "20332034", "20342035"
];

/// <summary>
/// The NHL season 1917-1918
/// </summary>
Expand Down
157 changes: 155 additions & 2 deletions Nhl.Api.Domain/Services/NhlGameService.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,169 @@
namespace Nhl.Api.Services;
using System.Collections.Generic;
using System.Globalization;
using System.Text.RegularExpressions;
using System;
using System.Threading.Tasks;
using Nhl.Api.Common.Http;
using Nhl.Api.Models.Game;
using System.Linq;
using Nhl.Api.Models.Schedule;

namespace Nhl.Api.Services;

/// <summary>
/// The NHL game service, enabling data enrichment and functionality to the Nhl.Api
/// </summary>
public interface INhlGameService
{

/// <summary>
/// A method to add the time of play for each play in the game center play by play
/// </summary>
/// <param name="gameCenterPlayByPlay"> A game center play by play object</param>
/// <returns>All of the game center play by play information with the time of play for each play</returns>
Task<GameCenterPlayByPlay> AddDateTimeOfPlayForEachPlay(GameCenterPlayByPlay gameCenterPlayByPlay);
}

/// <summary>
/// The NHL game service, enabling data enrichment and functionality to the Nhl.Api
/// </summary>
public class NhlGameService : INhlGameService
{
private readonly NhlScoresHtmlReportsApiHttpClient _nhlScoresHtmlReportsApiHttpClient = new();

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inject the HTTP client via dependency injection.

The _nhlScoresHtmlReportsApiHttpClient is instantiated directly within the NhlGameService class. This tight coupling makes unit testing more difficult and violates the Dependency Inversion Principle.

Consider injecting the NhlScoresHtmlReportsApiHttpClient through the constructor or using an interface to allow for better testability and adherence to SOLID principles.

/// <summary>
/// A method to add the estimated time of play for each play in the game center play by play
/// </summary>
/// <param name="gameCenterPlayByPlay"> A game center play by play object</param>
/// <returns>All of the game center play by play information with the time of play for each play</returns>
public async Task<GameCenterPlayByPlay> AddDateTimeOfPlayForEachPlay(GameCenterPlayByPlay gameCenterPlayByPlay)
{
var gameId = gameCenterPlayByPlay.Id.ToString(CultureInfo.InvariantCulture)[4..];
var scoreReport = await this._nhlScoresHtmlReportsApiHttpClient.GetStringAsync($"/{gameCenterPlayByPlay.Season}/PL{gameId}.HTM", default);

var regex = Regex.Matches(scoreReport, @"(?<=<td class="" \+ bborder"">)Period(.*?)(?=</td>)", RegexOptions.Compiled, TimeSpan.FromSeconds(5)).ToList();

if (string.IsNullOrWhiteSpace(scoreReport) || regex.Count == 0)
{
try
{
var gameStart = DateTimeOffset.Parse($"{gameCenterPlayByPlay.StartTimeUTC}", CultureInfo.InvariantCulture);
var timeSpanOffset = TimeSpan.Parse(gameCenterPlayByPlay.VenueUTCOffset, CultureInfo.InvariantCulture);

// Set offset
gameStart = gameStart.AddHours(timeSpanOffset.TotalHours);

// Set average delay in start time
gameStart = gameStart.Add(TimeSpan.FromMinutes(8.5));

var averageTimeDelayInIntermission = TimeSpan.FromMinutes(18);

// Average time of a period including stoppage
var timeOfPeriod = TimeSpan.FromMinutes(35);

foreach (var play in gameCenterPlayByPlay.Plays)
{
var period = play.Period;
var timeElapsed = TimeSpan.Zero;

if (period > 1)
{
var regularPeriodTime = period <= 3 ? ((period - 1) * timeOfPeriod) : TimeSpan.FromMinutes(105);
var averageDelayInIntermission = period <= 3 ? ((period - 1) * averageTimeDelayInIntermission) : TimeSpan.FromMinutes(60);
timeElapsed += averageDelayInIntermission + regularPeriodTime;
}

timeElapsed += TimeOnly.Parse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture).ToTimeSpan();
if (period > 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential parsing exceptions when converting time strings.

At line 75, parsing the time with TimeOnly.Parse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture) could throw a FormatException if play.TimeInPeriod is not in the expected format.

Use TimeOnly.TryParse to safely parse the time and handle any parsing errors appropriately. This ensures that invalid time formats do not cause unhandled exceptions.

Apply this diff to handle parsing safely:

-timeElapsed += TimeOnly.Parse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture).ToTimeSpan();
+if (TimeOnly.TryParse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture, out var parsedTime))
+{
+    timeElapsed += parsedTime.ToTimeSpan();
+}
+else
+{
+    // Handle parsing error (e.g., log or set a default value)
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
timeElapsed += TimeOnly.Parse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture).ToTimeSpan();
if (TimeOnly.TryParse($"00:{play.TimeInPeriod}", CultureInfo.InvariantCulture, out var parsedTime))
{
timeElapsed += parsedTime.ToTimeSpan();
}
else
{
// Handle parsing error (e.g., log or set a default value)
}

{
timeElapsed += period * TimeSpan.FromMinutes(10);
}

timeElapsed += TimeSpan.FromSeconds(5);
play.EstimatedDateTimeOfPlay = gameStart.Add(timeElapsed);
}
}
catch
{
}
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid silently swallowing exceptions in catch blocks.

In the catch block starting at line 85, exceptions are caught but no actions are taken. Silently swallowing exceptions can make debugging difficult and may hide underlying issues.

Consider logging the exception or rethrowing it to ensure that any errors are appropriately handled.

{
try
{
var startTimeOfEachPeriod = new Dictionary<int, List<DateTime>>
{
{ 1, [] },
{ 2, [] },
{ 3, [] },
{ 4, [] },
{ 5, [] },
};

for (var i = 0; i < regex.Count; i++)
{
var match = regex[i].Value;
var value = Regex.Match(match, @"([0-9]{1,2}:[0-9]{2})", RegexOptions.Compiled | RegexOptions.IgnoreCase, TimeSpan.FromSeconds(30)).Groups[0].Value;
var time = TimeOnly.Parse($"{value} PM", CultureInfo.InvariantCulture);
var dateTime = DateTime.Parse($"{gameCenterPlayByPlay.GameDate} {time} {gameCenterPlayByPlay.EasternUTCOffset}", CultureInfo.InvariantCulture);

if (i <= 1)
{
startTimeOfEachPeriod[1].Add(dateTime);
}
else if (i is >= 2 and <= 3)
{
startTimeOfEachPeriod[2].Add(dateTime);
}
else if (i is >= 4 and <= 5)
{
startTimeOfEachPeriod[3].Add(dateTime);
}
else if (i is >= 6 and <= 7)
{
startTimeOfEachPeriod[4].Add(dateTime);
}
else if (i <= 9)
{
startTimeOfEachPeriod[5].Add(dateTime);
}
}

var playsByPeriod = gameCenterPlayByPlay.Plays.GroupBy(x => x.Period).ToDictionary(x => x.Key, x => x.ToList());

foreach (var playsForPeriod in playsByPeriod)
{
var startTime = startTimeOfEachPeriod[playsForPeriod.Key].FirstOrDefault();
var endTime = startTimeOfEachPeriod[playsForPeriod.Key].LastOrDefault();
var distanceBetweenPlays = endTime - startTime;
var timeBetweenPlays = distanceBetweenPlays / playsForPeriod.Value.Count;

var multiplier = CalculateMultiplier(startTime, endTime, playsForPeriod.Value.Count);
for (var i = 0; i < playsForPeriod.Value.Count; i++)
{
var play = playsForPeriod.Value[i];
var timeElapsed = TimeOnly.Parse($"00:{playsForPeriod.Value[i].TimeInPeriod}", CultureInfo.InvariantCulture).ToTimeSpan();
play.EstimatedDateTimeOfPlay = startTime.Add(timeElapsed + (timeBetweenPlays * (i * multiplier)));
}
}
}
catch
{
}
}

return gameCenterPlayByPlay;
}

/// <summary>
/// A linear
/// </summary>
private double CalculateMultiplier(DateTime startTime, DateTime endTime, int events)
{
// Calculate duration in minutes
var duration = (endTime - startTime).TotalMinutes;

var multiplier = ((41 * duration) + (2 * events) - 381) / 3000;

return multiplier;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve documentation and extract magic numbers in CalculateMultiplier.

The method's documentation is incomplete, and the formula uses magic numbers without explanation.

 /// <summary>
-/// A linear 
+/// Calculates a multiplier for time estimation based on period duration and number of events.
+/// The multiplier is used to adjust the distribution of plays within a period.
 /// </summary>
+/// <param name="startTime">The start time of the period</param>
+/// <param name="endTime">The end time of the period</param>
+/// <param name="events">The number of events in the period</param>
+/// <returns>A multiplier value for time distribution calculations</returns>
 private double CalculateMultiplier(DateTime startTime, DateTime endTime, int events)
 {
+    // Constants for the multiplier formula
+    private const double DURATION_COEFFICIENT = 41.0;
+    private const double EVENTS_COEFFICIENT = 2.0;
+    private const double CONSTANT_TERM = 381.0;
+    private const double NORMALIZER = 3000.0;
+
     // Calculate duration in minutes
     var duration = (endTime - startTime).TotalMinutes;
 
-    var multiplier = ((41 * duration) + (2 * events) - 381) / 3000;
+    var multiplier = ((DURATION_COEFFICIENT * duration) + 
+                     (EVENTS_COEFFICIENT * events) - 
+                     CONSTANT_TERM) / NORMALIZER;
 
     return multiplier;
 }

Committable suggestion skipped: line range outside the PR's diff.

28 changes: 15 additions & 13 deletions Nhl.Api.Tests/GameTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -210,43 +210,45 @@ public async Task GetGameMetadataByGameIdAsync_Return_Valid_Information(int game
[TestMethodWithRetry(RetryCount = 5)]
public async Task NhlScoresHtmlReportsApiHttpClient_Can_Parse_Html_Report_For_Start_End_Period_Times()
{
var dictionary = new Dictionary<string, List<string>>
var dictionary = new Dictionary<string, List<TimeOnly>>
{
{ "P1", new List<string>() },
{ "P2", new List<string>() },
{ "P3", new List<string>() },
{ "OT", new List<string>() },
{ "SH", new List<string>() },
{ "P1", new List<TimeOnly>() },
{ "P2", new List<TimeOnly>() },
{ "P3", new List<TimeOnly>() },
{ "OT", new List<TimeOnly>() },
{ "SH", new List<TimeOnly>() },
};

var httpClient = new NhlScoresHtmlReportsApiHttpClient();
var gameReport = await httpClient.GetStringAsync("/20232024/PL020206.HTM");

var regex = Regex.Matches(gameReport, @"(?<=<td class="" \+ bborder"">)Period(.*?)(?=</td>)", RegexOptions.Compiled, TimeSpan.FromSeconds(30)).ToList();

for (int i = 0; i < regex.Count; i++)
for (var i = 0; i < regex.Count; i++)
{
var match = regex[i].Value;
var value = Regex.Match(match, @"([0-9]{1,2}:[0-9]{2}\s[A-Z]{3})", RegexOptions.Compiled | RegexOptions.IgnoreCase, TimeSpan.FromSeconds(30)).Groups[0].Value;
var value = Regex.Match(match, @"([0-9]{1,2}:[0-9]{2})", RegexOptions.Compiled | RegexOptions.IgnoreCase, TimeSpan.FromSeconds(30)).Groups[0].Value;
var time = TimeOnly.Parse($"{value} PM");
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve time parsing robustness

The current implementation assumes all times are PM, which may not always be correct. To improve robustness, consider the following approaches:

  1. If possible, extract the AM/PM information from the HTML report.
  2. Use game start time to determine the correct AM/PM for each period.
  3. Implement logic to handle time rollovers (e.g., from PM to AM for late-night games).

Example implementation:

var gameStartTime = ExtractGameStartTime(gameReport); // Implement this method
var time = ParseTimeWithContext(value, gameStartTime);

// ...

private TimeOnly ParseTimeWithContext(string timeString, TimeOnly gameStartTime)
{
    var parsedTime = TimeOnly.Parse(timeString);
    if (parsedTime < gameStartTime && gameStartTime.Hour >= 12)
    {
        // If parsed time is earlier than game start and game started in PM, assume next day
        parsedTime = parsedTime.AddHours(12);
    }
    return parsedTime;
}

This approach would provide more accurate time representations for games spanning different times of day.


if (i <= 1)
{
dictionary["P1"].Add(value);
dictionary["P1"].Add(time);
}
else if (i >= 2 && i <= 3)
{
dictionary["P2"].Add(value);
dictionary["P2"].Add(time);
}
else if (i >= 4 && i <= 5)
{
dictionary["P3"].Add(value);
dictionary["P3"].Add(time);
}
else if (i >= 6 && i <= 7)
{
dictionary["OT"].Add(value);
dictionary["OT"].Add(time);
}
else if (i <= 9)
{
dictionary["SH"].Add(value);
dictionary["SH"].Add(time);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling and validation

While the overall structure of the method is sound and the changes improve type safety, consider adding the following enhancements:

  1. Error handling for regex matches and time parsing.
  2. Validation of the extracted times (e.g., ensure they are in a logical sequence).
  3. Logging of any parsing issues or unexpected data.

Example:

try
{
    var time = TimeOnly.Parse($"{value} PM");
    if (time < previousTime)
    {
        // Log warning about unexpected time sequence
    }
    // ... existing code ...
}
catch (FormatException ex)
{
    // Log error about invalid time format
    // Consider how to handle this case (skip, use default value, etc.)
}

These additions would make the method more robust and easier to debug in case of unexpected data or parsing issues.

}
}
}
Expand Down
Loading
Loading