Report issue for duplicate journey patterns in NeTEx #5571
Conversation
habrahamsson-skanetrafiken
added
Improvement
NeTEx
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5571 +/- ##
=============================================
+ Coverage 67.21% 67.34% +0.13%
- Complexity 16086 16149 +63
=============================================
Files 1852 1857 +5
Lines 71003 71082 +79
Branches 7409 7404 -5
=============================================
+ Hits 47723 47872 +149
+ Misses 20813 20749 -64
+ Partials 2467 2461 -6 ☔ View full report in Codecov by Sentry. |
| tripPatternMapper | ||
| .mapTripPattern(journeyPattern) | ||
| .ifPresent(result -> { |
There was a problem hiding this comment.
Calling a none-trivial code block inside a function chain is not clean. Consider making a private method for the code block or using Java language features like if/for.
| TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPattern) { | ||
| // Make sure the result is clean, by creating a new object. | ||
| result = new TripPatternMapperResult(); | ||
| Optional<TripPatternMapperResult> mapTripPattern(JourneyPattern_VersionStructure journeyPattern) { |
There was a problem hiding this comment.
Observation: The original result class did handle the empty case, so this is weakening the existing functionality. The good thing is that a it is a bit less code to maintain, but since this does not reduce the code in the caller - it complicates it, than gain is minimal. I do not suggest to change it, but just wanted to mention it.
There was a problem hiding this comment.
I wanted to make it explicit that if we return a result then it will have a single TripPattern. I could have added an optional/nullable tripPattern field to the result class instead but that wouldn't really be simpler and i think this better expresses the actual behaviour of the mapTripPattern method.
Now the caller has to handle the case where the result is empty, it's a bit more complicated at the call site but IMO it's worth it.
There was a problem hiding this comment.
My point is that you are wrapping a result wrapper with another result wrapper - Optional and TripPatternMapperResult have some of the same responsibilities. You should not wrap the field inside TripPatternMapperResult with optional. We are return a collection here, not a single instance - you could provide a isEmpty() or ifPresent() method to make it explicit, but it was not needed; hence not added. The pattern we follow in OTP for collections is to return empty collections, not null or Optional.
There was a problem hiding this comment.
Another thing is that we avoid using records with arrays and collections. Arrays can not be protected, while collections are hard to protect - classes are simpler/less error prune. Even if the TripPatternMapperResult is just a DTO here, people have a tendency to copy code/and ways of doing things - so I want it to be clean everywhere - even if it in this case is ok/no risk.
There was a problem hiding this comment.
My point is that you are wrapping a result wrapper with another result wrapper - Optional and TripPatternMapperResult have some of the same responsibilities.
In this PR my idea is to change the responsibilities so that the result class is only responsible for representing an actual result. The Optional is responsible for representing presence/absence of a result. I think this is a pretty common pattern. And I think it is a good pattern since the result class would only have to concern itself with a single responsibility.
We are return a collection here, not a single instance - you could provide a
isEmpty()orifPresent()method to make it explicit, but it was not needed; hence not added. The pattern we follow in OTP for collections is to return empty collections, notnullor Optional.
The thing is that I want the result to have a single instance of a TripPattern instead of a collection of TripPatterns. Because I want the caller to be able to see what the resulting id is and I wanted it to be obvous that the issue for duplicated ids is only reported once.
In this codebase, would it be more idiomatic to put a List<TripPattern> in the result and have it be implicit that this is either empty or has a single entry?
There was a problem hiding this comment.
Another thing is that we avoid using records with arrays and collections. Arrays can not be protected, while collections are hard to protect - classes are simpler/less error prune. Even if the
TripPatternMapperResultis just a DTO here, people have a tendency to copy code/and ways of doing things - so I want it to be clean everywhere - even if it in this case is ok/no risk.
Is this in order to protect the collections from mutation? I think that's a good concern. But then having collections as public attributes should be just as bad, or am I missing something? In this refactored code it wouldn't be that hard to make the collections unmodifiable when creating the record to protect them from modification. It's not perfect but I think it's a win for this style.
There was a problem hiding this comment.
In this codebase, would it be more idiomatic to put a List in the result and have it be implicit that this is either empty or has a single entry?
Of cause not. The TripPatternMapperResult has a single-responsibility - to carry the result of one method. The result has many different states, which you toke just one out.
But we can agree to disagree - that is fine. I will approve the PR.
habrahamsson-skanetrafiken
merged commit aaaedbe
into
opentripplanner:dev-2.x
Summary
As a follow-up to #5552, this PR reports an issue to the issueStore if there are duplicate journeypatterns in the Netex data.
Unit tests
Had to do some minor touches to the TripPatternMapperTest.
Bumping the serialization version id
No