Skip to content

Add Smoove citybikes updater - #2515

Merged
t2gran merged 1 commit into
opentripplanner:masterfrom
HSLdevcom:smoove-citybikes
Aug 20, 2018
Merged

t2gran merged 1 commit into
opentripplanner:masterfrom
HSLdevcom:smoove-citybikes

Conversation

@hannesj

@hannesj hannesj commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

This PR adds a new updater for Smoove bike rental systems.

Example api endpoint is http://helsinki-fi.smoove.pro/api-public/stations

@hannesj
hannesj force-pushed the smoove-citybikes branch 4 times, most recently from 291d83a to b90fb0f Compare December 7, 2017 13:43

@abyrd abyrd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This being an optional plugin, it has little chance of interfering with the rest of the OTP system, and it's bundled with tests, all good signs. Ongoing maintenance of this code will be up to your team @hannesj.

@abyrd
abyrd requested a review from a team July 20, 2018 04:47

@t2gran t2gran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 100% test coverage
👎 Minor code improvement, but not a blocker from my side.

return null;
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a bit of code duplication node.path("coordinates").asText().split(",") and the try-catch-block is unnecessary wide. The comment on line 69 becomes obsolete if the try catch is narrowed:

    public BikeRentalStation makeStation(JsonNode node) {
        BikeRentalStation station = new BikeRentalStation();
        station.id = node.path("name").asText().split("\\s", 2)[0];
        station.name = new NonLocalizedString(node.path("name").asText().split("\\s", 2)[1]);
        try {
            String[] coordinatesAsString = node.path("coordinates").asText().split(",");
            station.y = Double.parseDouble(coordinatesAsString[0].trim());
            station.x = Double.parseDouble(coordinatesAsString[1].trim());
        } catch (NumberFormatException e) {
            log.warn("Error parsing bike rental station " + station.id, e);
            return null;
        }
        if (!node.path("operative").asText().equals("true")) {
            station.bikesAvailable = 0;
            station.spacesAvailable = 0;
        } else {
            station.bikesAvailable = node.path("avl_bikes").asInt();
            station.spacesAvailable = node.path("free_slots").asInt();
        }
        return station;
    }
``
`

@hannesj

hannesj commented Aug 20, 2018 •

Copy link
Copy Markdown
Contributor Author

@t2gran I updated the code according to your comments. Would it be possible to merge?

@t2gran
t2gran merged commit 15232bd into opentripplanner:master Aug 20, 2018
@vesameskanen
vesameskanen deleted the smoove-citybikes branch November 24, 2022 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants