Cleanup regular transfer build parameters#7425
Cleanup regular transfer build parameters#7425t2gran merged 5 commits intoopentripplanner:dev-2.xfrom
Conversation
…update references
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7425 +/- ##
=============================================
+ Coverage 71.12% 71.29% +0.16%
- Complexity 20895 20971 +76
=============================================
Files 2332 2332
Lines 86631 86710 +79
Branches 8579 8581 +2
=============================================
+ Hits 61615 61818 +203
+ Misses 22038 21897 -141
- Partials 2978 2995 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
leonardehrenfried
left a comment
There was a problem hiding this comment.
There is a tiny typo, but this looks like a good simplification.
VillePihlava
left a comment
There was a problem hiding this comment.
Looks good, I just found a naming issue
| private final Duration maxDuration; | ||
| private final Map<StreetMode, TransferParametersForMode> parametersForMode; | ||
| private final List<RouteRequest> requests; |
There was a problem hiding this comment.
Although I think the renaming to e.g. maxDuration makes sense, now there is a discrepancy between the names in TransferParametersForMode, i.e.
maxTransferDuration
carsAllowedStopMaxTransferDuration
bikesAllowedStopMaxTransferDuration
I think for the sake of clarity, maybe we should leave the word transfer in the names. What do you think? Also its possible that someone adds a maximum duration to the transfer config that isn't the maximum transfer duration.
There was a problem hiding this comment.
I think we should rename the names in TransferParametersForMode and follow defacto OOP naming conventions. I can do that in this PR if you want.
There was a problem hiding this comment.
I would want them to be consistent, but I would prefer leaving the transfer as part of the name. For example, carsAllowedStopMaxTransferDuration is clearer than carsAllowedStopMaxDuration, or disableDefaultTransfers is clearer than disableDefaults.
This is not that important for me so we can also just leave them as is, or you can name them to what suits you
There was a problem hiding this comment.
Please don´t do that. We use OOP and it should be DRY (Contextual Naming). This apply to APIs as well. If the context is not clear, you may consider repeating it or if something could exist outside the context.
864b8b7 to
4987f88
Compare
Summary
This is a pure refactoring of the transfer-related build configuration parameters. Three previously separate build config concerns —
maxTransferDuration,transferRequests, andtransferParametersForMode— are grouped into a singleRegularTransferParametersvalue object, making the API surface ofBuildConfigandDirectTransferGeneratorcleaner and cohesive. No behavioral changes are introduced.This PR does not change the configuration. A similar refactoring to group transfer parameters in the configuration is welcome, but I will not do for now.
I have used the new name RegularTransfer, and not the old DirectTransfer for everything I have changed. A refactoring to change DirectTransfer to RegularTransfer would be nice, but that is a large refactoring.
Refactoring
New parameters class:
RegularTransferParametersA new immutable value object
RegularTransferParameters(with aBuilder) groups the three related transfer build parameters:maxDuration(formerlymaxTransferDurationonBuildConfig)parametersForMode(formerlytransferParametersForModeonBuildConfig)requests(formerlytransferRequestsonBuildConfig)BuildConfignow exposes a singleregularTransferParameters()accessor instead of three public fields, improving encapsulation.Renamed and relocated types
graph_builder/module/TransferParametersgraph_builder/module/transfer/api/TransferParametersForModeconfig/buildconfig/TransferParametersMapperconfig/buildconfig/RegularTransferConfigconfig/buildconfig/TransferConfigRegularTransferConfig)config/buildconfig/TransferRequestConfigRegularTransferConfig)The new names better reflect their roles:
TransferParametersForModeclarifies that this holds per-mode parameters (not all transfer parameters).RegularTransferConfigconsolidates all regular-transfer JSON config mapping in one place.Simplified
DirectTransferGeneratorconstructorThe constructor previously accepted three separate parameters (
Duration,List<RouteRequest>,Map<StreetMode, TransferParameters>). It now accepts a singleRegularTransferParametersinstance, reducing call sites from 3 arguments to 1.Testing
Unit Tests
Documentation
Changelog
Bumping the serialization version id