Jormun: Fix journey's sections sorter #3894
Conversation
| if a.begin_date_time == b.begin_date_time and a.end_date_time == b.end_date_time: | ||
| return -1 if a.destination.uri == b.origin.uri else 1 | ||
| if a.begin_date_time != b.begin_date_time: | ||
| return -1 if a.begin_date_time < b.begin_date_time else 1 | ||
| else: |
There was a problem hiding this comment.
Smart to try to join sections 💡
But how about 3 sections having the same values, or 2 "distant" sections? Maybe raising would be better 🤷
Anyway, more efficient (and clearer IMO):
if a.begin_date_time != b.begin_date_time:
return -1 if a.begin_date_time < b.begin_date_time else 1
elif a.end_date_time != b.end_date_time:
return -1 if a.end_date_time < b.end_date_time else 1
elif a.destination.uri == b.origin.uri:
return -1
elif a.origin.uri == b.destination.uri:
return 1
else: # still not decided, but default to something stable. Or is there a return 0 possible ?
return -1 if a.origin.uri < b.origin.uri else 1 # or should we explicitly raise?
There was a problem hiding this comment.
I like your proposition better, it's clearer.
However, about you last comment, if I'm not wrong, I wouldn't recommend to return 0 (even if it's probably possible). We want a stable sorting and depending on the sorting algorithm, the original order might or might not be preserved.
By the way, this class is called SectionSorter, but I don't think the naming is correct, it doesn't sort, it only compares. How about renaming it SectionComparator?
There was a problem hiding this comment.
Now I am wondering why we need to sort sections 🤔
There was a problem hiding this comment.
@pbench In the protobuf you cannot add a section at the beginning
There was a problem hiding this comment.
@woshilapin returning a raw value in the else case (no matter the value) is not stable, as it will depend on which is left or right member. Sorting on uri is stable but not relevant... I guess raising has my preference, but it's not strong.
✔️ Anyway, it's quite clear and way better now, so it's fine by me.
Naming proposition by @woshilapin is better but not mandatory IMO.
@pbench avoid that sorting by a stitching mechanism of section-groups would be more correct, but this is more work, so I'm OK to keep this.
|
Kudos, SonarCloud Quality Gate passed! |
| return -1 | ||
| elif a.origin.uri == b.destination.uri: | ||
| return 1 | ||
| else: |
There was a problem hiding this comment.
I wonder if we should put a log in this else case since it's not supposed to happen. Might help to debug in case it actually happens.
There was a problem hiding this comment.
Indeed, logging (warning or error) here would be nice 👍
Jira: NAV-1678
The sort of sections is random when two sections have the same departure_date_time and arrival_date_time
Before correction:
