Conversation
…flow Co-authored-by: simonech <61557+simonech@users.noreply.github.com>
| _parameters.ClimbGrade = routeSegment.AverageGradient * 100; | ||
|
|
||
| double speedKmh = calculator.CalculateVelocity(powerWatts); |
There was a problem hiding this comment.
I don't like that parameters are mutated.
Can you refactor it into
double CalculateVelocity(double power, double gradient)
Also remove the climb grade parameter from the parameter object.
And while we are here:
The old parameter was a percentage, so it was divided by 100 before calculating the Atan.
The grade from the segment is already a decimal. So harmonise by always using a decimal without converting to and from percentage.
There was a problem hiding this comment.
Done in 929c1de:
ClimbGraderemoved fromParameters- Added
CalculateVelocity(double power, double gradient)(and a matchingCalculatePoweroverload + privateCalculateForcestaking gradient directly) - Gradient is now always a decimal throughout —
Math.Atan(gradient)with no/ 100anywhere RideCalculatorpassesrouteSegment.AverageGradientdirectly to the new overload; no more mutation
…from Parameters Co-authored-by: simonech <61557+simonech@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements a speed and time calculation feature for cycling routes by integrating physics-based velocity calculations with route segment data. It addresses issue #7 by creating a ride model that combines route segments with calculated speed and duration based on rider power input.
Changes:
- Refactored
VirtualSpeedCalculatorto accept gradient as an explicit parameter instead of using aClimbGradeproperty, harmonizing gradient representation as decimal values throughout - Added
RideCalculatorservice to compute ride segments with speed, duration, and power for each route segment - Created
RideSegmentmodel to represent ride data (duration, speed, power) with a reference to the corresponding route segment - Enhanced CLI to require
--powerargument and display total time and average speed in addition to existing metrics - Updated CI workflow to pass
--power 200and verify new output fields
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| VirtualSpeed/VirtualSpeed/VirtualSpeedCalculator.cs | Refactored to pass gradient as method parameter; removed ClimbGrade from Parameters class; added overloads for backward compatibility |
| VirtualSpeed/VirtualSpeed/Services/RideCalculator.cs | New service that calculates ride segments by computing velocity for each route segment based on power and gradient |
| VirtualSpeed/VirtualSpeed/Model/RideSegment.cs | New immutable record type containing duration, speed, power, and route segment reference |
| VirtualSpeed/VirtualSpeed/Program.cs | Added --power argument parsing; integrated RideCalculator; added total time and average speed to output |
| .github/workflows/build.yml | Updated test command to include --power 200 and verify Avg Speed appears in output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int i = 0; i < args.Length - 1; i++) | ||
| { | ||
| Console.WriteLine("Usage: app.exe --gpx <route.gpx>"); | ||
| return; | ||
| if (args[i] == "--gpx") | ||
| filePath = args[i + 1]; | ||
| else if (args[i] == "--power") | ||
| { | ||
| if (!double.TryParse(args[i + 1], System.Globalization.NumberStyles.Any, | ||
| System.Globalization.CultureInfo.InvariantCulture, out power)) | ||
| { | ||
| Console.WriteLine("Error: --power must be a valid number."); | ||
| return; | ||
| } | ||
| hasPower = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The argument parsing loop will skip the last argument if it's a flag. The condition i < args.Length - 1 means if someone passes --gpx file.gpx --power (without a value), the --power flag won't be checked. Additionally, if --power is the last argument with a value, it will work, but if --gpx is the last argument, it won't be processed. Consider changing the loop to check i < args.Length and then verify that i + 1 < args.Length before accessing args[i + 1] to provide better error handling.
| @@ -81,7 +91,7 @@ public double CalculateVelocity(double power) | |||
| lowervel = midvel; | |||
|
|
|||
| midvel = (uppervel + lowervel) / 2.0; | |||
| midpow = CalculatePower(midvel); | |||
| midpow = CalculatePower(midvel, gradient); | |||
| } while (itcount++ < 100); | |||
|
|
|||
| return midvel; | |||
There was a problem hiding this comment.
The binary search for velocity starts with a range of -1000.0 to 1000.0 km/h, which could converge to a negative velocity for steep downhills with low power input. While the RideCalculator checks speedMs > 0 and sets duration to 0 in that case, this is a physically unrealistic scenario. Consider adding validation to ensure power is sufficient for the route, or clamping negative velocities to a minimum realistic speed, or providing a warning when this occurs.
| { | ||
| Console.WriteLine("Error: --power must be a valid number."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The power argument is parsed but not validated for positive values. A negative or zero power value would be physically meaningless and could cause issues in the velocity calculation. Consider adding validation to ensure power is a positive number (e.g., power <= 0) and providing a clear error message if it's not.
| } | |
| } | |
| if (power <= 0) | |
| { | |
| Console.WriteLine("Error: --power must be a positive number (watts)."); | |
| return; | |
| } |
| output=$(dotnet run --no-build --configuration Release -- --gpx routeWithDownhill.gpx --power 200) | ||
| echo "$output" | ||
| if echo "$output" | grep -q "Total Distance:"; then | ||
| if echo "$output" | grep -q "Total Distance:" && echo "$output" | grep -q "Avg Speed:"; then |
There was a problem hiding this comment.
The workflow test checks for "Total Distance:" and "Avg Speed:" in the output, but doesn't verify "Total Time:" which is also a new output added in this PR. Consider adding a check for "Total Time:" to ensure all new outputs are being generated correctly.
| if echo "$output" | grep -q "Total Distance:" && echo "$output" | grep -q "Avg Speed:"; then | |
| if echo "$output" | grep -q "Total Distance:" && echo "$output" | grep -q "Avg Speed:" && echo "$output" | grep -q "Total Time:"; then |
Model/RideSegment.cswith duration, speed, power, and route segment referenceServices/RideCalculator.csto calculate ride segments using physics formulaProgram.csto accept--powerargument, use RideCalculator, output Total Time + Avg Speed.github/workflows/build.ymlto pass--power 200and verify new outputCalculateVelocity(double power, double gradient)overload — gradient passed as decimal, no mutation of ParametersClimbGradefromParameters; gradient now flows as an explicit method argument/ 100conversions)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.