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.

Sign up for GitHub

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

Jump to bottom

Rename all methods in TransitService according to naming conventions #6249

Merged
merged 7 commits into from
Nov 28, 2024
Merged

Rename all methods in TransitService according to naming conventions #6249

merged 7 commits into from
Nov 28, 2024

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Nov 13, 2024 •
edited
Loading

Summary

We have had naming conventions for a while but many central pieces don't actually adhere to it. This PR changes all names in TransitService to use the conventions.

I made two small functional changes:

  • the method getScheduledTrip was moved to TransitEditorService
  • unused methods were removed

Issue

#6048

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner November 13, 2024 12:17
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 37.44681% with 147 lines in your changes missing coverage. Please review.

Project coverage is 69.75%. Comparing base (669d71f) to head (aacd1d7).
Report is 8 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...pentripplanner/ext/restapi/resources/IndexAPI.java 0.00% 20 Missing ⚠️
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 22.22% 14 Missing ⚠️
...anner/apis/transmodel/TransmodelGraphQLSchema.java 6.66% 14 Missing ⚠️
...ntripplanner/apis/gtfs/datafetchers/AlertImpl.java 0.00% 11 Missing ⚠️
...entripplanner/apis/gtfs/datafetchers/StopImpl.java 16.66% 10 Missing ⚠️
...lanner/apis/transmodel/model/network/LineType.java 55.55% 8 Missing ⚠️
...nner/apis/transmodel/model/stop/StopPlaceType.java 0.00% 8 Missing ⚠️
...ner/apis/transmodel/model/siri/sx/AffectsType.java 0.00% 6 Missing ⚠️
...ansmodel/model/siri/sx/PtSituationElementType.java 0.00% 6 Missing ⚠️
.../opentripplanner/routing/TripTimeOnDateHelper.java 0.00% 4 Missing ⚠️
... and 31 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             dev-2.x    #6249   +/-   ##
==========================================
  Coverage      69.75%   69.75%           
+ Complexity     17752    17750    -2     
==========================================
  Files           2011     2011           
  Lines          75980    75969   -11     
  Branches        7784     7784           
==========================================
- Hits           52999    52995    -4     
+ Misses         20277    20269    -8     
- Partials        2704     2705    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

t2gran
t2gran previously approved these changes Nov 19, 2024
View reviewed changes
@leonardehrenfried
Copy link
Member Author

There were merge conflicts so I had to merge dev-2.x.

@optionsome optionsome self-requested a review November 19, 2024 10:41
t2gran
t2gran previously approved these changes Nov 19, 2024
View reviewed changes
optionsome
optionsome requested changes Nov 20, 2024
View reviewed changes
application/src/main/java/org/opentripplanner/transit/service/TransitService.java
Collection<Agency> getAgencies();
Optional<Agency> findAgencyById(FeedScopedId id);
Collection<Agency> listAgencies();
Optional<Agency> findAgency(FeedScopedId id);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be getAgency?

Copy link
Member Author

Choose a reason for hiding this comment

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

It returns an optional, so that's why it's find.

application/src/main/java/org/opentripplanner/transit/service/TransitService.java

FeedInfo getFeedInfo(String feedId);

Collection<Notice> getNoticesByEntity(AbstractTransitEntity<?, ?> entity);
Collection<Notice> findNotices(AbstractTransitEntity<?, ?> entity);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit on the edge if we can use the standard findX convention for this method. In general, I think there are a lot of cases where it would improve the readability a bit to explains in the method name how the argument is used for the search. I would prefer findNoticesForEntity in this case. We can discuss this in a dev meeting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning a collection means find

application/src/main/java/org/opentripplanner/transit/service/TransitService.java

/**
* Return the routes using the given stop, not including real-time updates.
*/
Set<Route> getRoutesForStop(StopLocation stop);
Set<Route> findRoutes(StopLocation stop);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, it returns a collection, that's why find.

application/src/main/java/org/opentripplanner/transit/service/TransitService.java
Comment on lines -147 to +143
Collection<StopLocation> getStopOrChildStops(FeedScopedId id);
Collection<StopLocation> findStopOrChildStops(FeedScopedId id);
Copy link
Member

Choose a reason for hiding this comment

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

My intuition here would be to use get because the argument is an id.

Copy link
Member Author

Choose a reason for hiding this comment

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

Collection is find

application/src/main/java/org/opentripplanner/transit/service/TransitService.java Outdated
Comment on lines 224 to 215
Collection<Route> getRoutesForGroupOfRoutes(GroupOfRoutes groupOfRoutes);
Collection<Route> findGroupsOfRoutes(GroupOfRoutes groupOfRoutes);
Copy link
Member

Choose a reason for hiding this comment

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

I think the name should be findRoutes but I prefer a longer version like findRoutesForGroupOfRoutes.

Copy link
Member Author

Choose a reason for hiding this comment

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

5bf1fc0

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the short version, because the argument is part of the method signature - DRY

application/src/main/java/org/opentripplanner/transit/service/TransitService.java Outdated

/**
* Return the TripOnServiceDate for a given id, including real-time updates.
*/
TripOnServiceDate getTripOnServiceDateById(FeedScopedId datedServiceJourneyId);
TripOnServiceDate findTripOnServiceDate(FeedScopedId datedServiceJourneyId);
Copy link
Member

Choose a reason for hiding this comment

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

This is again a bit on the edge, get or find?

Copy link
Member Author

Choose a reason for hiding this comment

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

5bf1fc0

Copy link
Member

Choose a reason for hiding this comment

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

The argument name is old, it should be tripOnServiceDateId/idnot datedServiceJourneyId. Again I prefear id because it is DRY.

Copy link
Member Author

Choose a reason for hiding this comment

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

8a9b964

t2gran
t2gran previously approved these changes Nov 27, 2024
View reviewed changes
optionsome
optionsome previously approved these changes Nov 28, 2024
View reviewed changes
@leonardehrenfried leonardehrenfried merged commit 996fa3f into opentripplanner:dev-2.x Nov 28, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the naming-conventions branch November 28, 2024 12:09
@leonardehrenfried
Copy link
Member Author

leonardehrenfried commented Nov 28, 2024 •
edited
Loading

The conflict was only about imports so I used my admin rights to merge even though your reviews were dismissed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@optionsome optionsome optionsome left review comments

@t2gran t2gran t2gran left review comments

Assignees
No one assigned
Labels
Skip Changelog Technical Debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leonardehrenfried @t2gran @optionsome