ENH: NAV-52 - First throw at defining interfaces for a public transport interface. #19
Conversation
…e, Trip and Stop interfaces.
There was a problem hiding this comment.
Thanks, nice work @clukas1! I think this is a well structured interface for our service 👍
I added quite a lot of comments, but they are more about details and conventions:
- Remove redundancy in names (getRouteId -> getId) member variables (Locations and Stops)
- Only add fully specified methods (so no overloads with defaults for paramters), since then it is in the control of the implementing class, which I think is not what we want.
- NotNull, maybe we should set this as default and only use Nullable? Otherwise it gets with verbose.
Something I am not sure yet what a good solution could be is the differentiation between trip / route and walking legs. Somehow I don't like the nullable members.
I will checkout a new branch review/NAV-52 and refactor my suggestions, and add a PR for your review.
| import java.time.LocalDate; | ||
|
|
||
| public interface ArrivalTime { | ||
| @NotNull LocalDate getArrivalTime(); |
There was a problem hiding this comment.
Should we introduce / use this annotation? It gets quite verbose...
Until now we just marked @nullable fields and assumed that fields without annotations are never null.
|
|
||
| import java.time.LocalDate; | ||
|
|
||
| public interface ArrivalTime { |
There was a problem hiding this comment.
Why do we need this? LocalDateTime and the variable name arrival or departure time should be sufficient in my opinion.
| import java.time.LocalDate; | ||
|
|
||
| public interface DepartureTime { | ||
| @NotNull LocalDate getDepartureTime(); |
There was a problem hiding this comment.
Again, I would not wrap a single data type. We can achieve clarity here by using the variable name "departueTime". Or is there a benefit I don't see yet?
| public interface Leg { | ||
| @NotNull Location getDepartureLocation(); | ||
|
|
||
| @NotNull Location getArrivalLocation(); |
There was a problem hiding this comment.
I would not return the location separately... the location can be retrieved via the stop.getLocation()
There was a problem hiding this comment.
A leg can be from a random coordinate to the nearest stop. So there are cases where stop will be null
|
|
||
| @Nullable Stop getArrivalStop(); | ||
|
|
||
| @NotNull DepartureTime getDepartureTime(); |
There was a problem hiding this comment.
Just use normal LocalTime here?
There was a problem hiding this comment.
This was mainly an idea to allow method overrides for getConnections() / earliestArrivals() specifying the direction of travel. But this could of course also be moved to the QueryConfig interface.
| @@ -0,0 +1,8 @@ | |||
| package ch.naviqore.service; | |||
|
|
|||
| public enum SearchType { | |||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| public interface Stop extends Location { | ||
| @NotNull String getStopId(); |
|
|
||
| @NotNull Stop getStop(); | ||
|
|
||
| @NotNull ArrivalTime getArrivalTime(); |
|
|
||
| @NotNull Route getRoute(); | ||
|
|
||
| @NotNull StopTime getStartStop(); |
There was a problem hiding this comment.
This is redundant, since information can be retrieved via stop times.
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public interface Walk { | ||
| @NotNull Location getStart(); |
There was a problem hiding this comment.
Redundant, since can be retrieved via Stop. Except we support walks from random locations to stops. E.g. from an address to the station. But I would keep it simple and extend the interface if we need it.
- Rename to project consistent names: Nearest, connection, source and target. - Remove methods that allow default parameters in the implementation. - Remove redundant information in the interface which can be retrieved from member objects further down.
|
Please see #21 |
- Format project.
…for-service REFACTOR: NAV-52 - Remove redundancy in public transit service interface
Proposal for a general interface for the Public Transport Service Interface