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.

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

23 service improvements #50

Merged
merged 13 commits into from
Aug 25, 2023
Merged

23 service improvements #50

merged 13 commits into from
Aug 25, 2023

Conversation

Kebslock
Copy link
Contributor

This PR is mainly for complete review of the POI-, Track- and Vehicle-Service (because it was not done so far).

Furthermore there were changes to improve handling of GeoJSON data and also some minor code improvements.

@Kebslock Kebslock added enhancement New feature or request backend labels Jul 30, 2023
@Kebslock Kebslock self-assigned this Jul 30, 2023
Copy link
Member

@NiklasRentzCAU NiklasRentzCAU left a comment

Choose a reason for hiding this comment

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

This looks good so far with the individual services and common utils on the side for all of them. Looking through the code comments: have you decided yet if you want to use / which logging framework you want to use? Answering this could solve a lot of the TODOs left in the code.
Good use of TODOs to keep track of minor things in the code to be replaced as the project progresses.
Also make sure to add a copyright/license header to your files, see my comment on the other PRs.

@@ -97,11 +104,17 @@ export default class POIService{
*/
public static async getPOITrackDistanceKm(poi: POI): Promise<number | null>{
// get closest track if none is given
const poiPos: GeoJSON.Feature<GeoJSON.Point> = JSON.parse(JSON.stringify(poi.position))
if (poiPos == null || poiPos.properties == null || poiPos.properties["trackKm"] == null) {
const poiPos = GeoJSONUtils.parseGeoJSONFeaturePoint(JSON.parse(JSON.stringify(poi.position)))
Copy link
Member

Choose a reason for hiding this comment

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

stringifying and immediately parsing an object seems unnecessary/waseful. I guess you only want an object with the correct type for you to work on it or is there a reason why you would not want to work on the poi.position directly?

@Kebslock Kebslock marked this pull request as draft August 4, 2023 15:57
}
allVehiclesOnTrack.filter(async function (vehicle, index, vehicles){
const vehicleTrackKm = await VehicleService.getVehicleTrackDistanceKm(vehicle)
if (vehicleTrackKm == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an error but more like an inconsistency:
Sometimes you use the ?? (Nullish Coalescing) operator to either return the "right" value or if it is "null" another value, and then sometimes you use a if statement.

@Niatsuna Niatsuna merged commit 1548d6b into development Aug 25, 2023
1 check passed
@Niatsuna Niatsuna deleted the 23-service-improvements branch August 25, 2023 17:46
@n1kPLV
Copy link
Contributor

n1kPLV commented Aug 25, 2023

This clearly needs a little bit more work. Please adjust call sites when changing the signature of functions:

> tsc && node ./build/index.js

src/routes/vehicle.route.ts(358,105): error TS2554: Expected 1 arguments, but got 2.
src/routes/vehicle.route.ts(371,100): error TS2554: Expected 1 arguments, but got 2.

@n1kPLV
Copy link
Contributor

n1kPLV commented Aug 25, 2023

Also there is infinite recursion when calculating a vehicle position. getVehiclePosition() calls getVehicleTrackHeading(), which in turn calls getVehicleTrackDistanceKm(), which calls getVehiclePosition() again.

I am considering reverting this merge.

@n1kPLV n1kPLV restored the 23-service-improvements branch August 26, 2023 08:12
n1kPLV added a commit that referenced this pull request Aug 26, 2023
This reverts commit 1548d6b, reversing
changes made to e07af0f.
@n1kPLV n1kPLV mentioned this pull request Aug 26, 2023
n1kPLV added a commit that referenced this pull request Aug 26, 2023
This reverts commit 1548d6b, reversing
changes made to e07af0f.
@n1kPLV n1kPLV mentioned this pull request Aug 26, 2023
@Kebslock Kebslock deleted the 23-service-improvements branch August 30, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants