Skip to content

fix(OFJAVA-013): 3 review findings in CityService.java #73

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

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −28
Draft

fix(OFJAVA-013): 3 review findings in CityService.java #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 23 additions & 28 deletions backend/src/main/java/cx/flamingo/analysis/service/CityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.csv.CSVFormat;
import org.apache.commons.csv.CSVParser;
import org.apache.commons.csv.CSVRecord;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Lazy;
import org.springframework.core.io.ClassPathResource;
Expand Down Expand Up @@ -42,23 +46,19 @@ public void init() {
private void loadCities() {
cities = new ArrayList<>();
try (BufferedReader reader = new BufferedReader(
new InputStreamReader(new ClassPathResource("data/cities.csv").getInputStream()))) {

// Skip header
reader.readLine();

String line;
while ((line = reader.readLine()) != null) {
String[] parts = line.split(",");
String id = parts[0];
String name = parts[1];
String stateId = parts[2];
int population = Integer.parseInt(parts[3]);
double latitude = Double.parseDouble(parts[4]);

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 CityService CSV parser splits on comma without handling quoted fields, breaking city names with commas

In loadCities(), replaced the BufferedReader+line.split(",") approach with Apache Commons CSV (CSVParser / CSVRecord) to correctly handle quoted fields containing commas. The CSVFormat.DEFAULT.withFirstRecordAsHeader() replaces the manual reader.readLine() header skip, and column values are accessed by index via record.get(N). Risk: Apache Commons CSV (commons-csv) must be present as a dependency in the project's pom.xml / build.gradle — if it is not already declared, the build will fail and the dependency must be added. The column index mapping (0–6) is assumed to match the original parts[0]–parts[6] ordering exactly. A reviewer should verify the CSV header order and confirm the dependency is available.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/CityService.java around line 57, review and complete this code-review fix: CityService CSV parser splits on comma without handling quoted fields, breaking city names with commas.
What the draft fix changed: In `loadCities()`, replaced the `BufferedReader`+`line.split(",")` approach with Apache Commons CSV (`CSVParser` / `CSVRecord`) to correctly handle quoted fields containing commas. The `CSVFormat.DEFAULT.withFirstRecordAsHeader()` replaces the manual `reader.readLine()` header skip, and column values are accessed by index via `record.get(N)`. Risk: Apache Commons CSV (`commons-csv`) must be present as a dependency in the project's `pom.xml` / `build.gradle` — if it is not already declared, the build will fail and the dependency must be added. The column index mapping (0–6) is assumed to match the original `parts[0]`–`parts[6]` ordering exactly. A reviewer should verify the CSV header order and confirm the dependency is available.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

double longitude = Double.parseDouble(parts[5]);
Set<String> regionIds = Arrays.stream(parts[6].split("\\|"))
new InputStreamReader(new ClassPathResource("data/cities.csv").getInputStream()));
CSVParser csvParser = new CSVParser(reader, CSVFormat.DEFAULT.withFirstRecordAsHeader())) {

for (CSVRecord record : csvParser) {
String id = record.get(0);
String name = record.get(1);
String stateId = record.get(2);
int population = Integer.parseInt(record.get(3));
double latitude = Double.parseDouble(record.get(4));
double longitude = Double.parseDouble(record.get(5));
Set<String> regionIds = Arrays.stream(record.get(6).split("\\|"))
.collect(Collectors.toSet());

City city = City.builder()
.id(id)
.name(name)
Expand All @@ -68,10 +68,10 @@ private void loadCities() {
.longitude(longitude)
.regionIds(regionIds)
.build();

// Set nearest team ID
city.setNearestTeamId(soccerTeamService.findNearestTeamId(city));

cities.add(city);
}
} catch (IOException e) {
Expand Down Expand Up @@ -128,13 +128,11 @@ public List<City> getCitiesByRegionId(String regionId) {
.collect(Collectors.toList());
}

public City getCityById(String id) {
City city = cities.stream()
public Optional<City> getCityById(String id) {
return cities.stream()
.filter(c -> c.getId().equals(id))
.findFirst()
.orElse(null);

return city != null ? populateState(city) : null;

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 CityService.getCitiesByNearestTeamId() recomputes nearest team for every city on every call

In getCitiesByNearestTeamId (line 137), replaced soccerTeamService.findNearestTeamId(city) inside the stream filter with city.getNearestTeamId(), using the value already stored on the city during loadCities(). This is a direct mechanical fix with no behavioral change for correct data, and eliminates the O(n*m) recomputation on every call.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/CityService.java around line 137, review and complete this code-review fix: CityService.getCitiesByNearestTeamId() recomputes nearest team for every city on every call.
What the draft fix changed: In `getCitiesByNearestTeamId` (line 137), replaced `soccerTeamService.findNearestTeamId(city)` inside the stream filter with `city.getNearestTeamId()`, using the value already stored on the city during `loadCities()`. This is a direct mechanical fix with no behavioral change for correct data, and eliminates the O(n*m) recomputation on every call.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

.map(this::populateState);
}

public List<City> getAllCities() {
Comment on lines 128 to 138

@flamingo flamingo Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 CityService.getCityById() returns null instead of throwing or returning Optional

In getCityById (line 115), changed the return type from City to Optional<City> and rewrote the method body to return cities.stream()...findFirst().map(this::populateState), eliminating all null returns. The Optional import was added. Risk: callers in EntityController (and any other callers not visible in this file) that currently do a null-check on the returned City will now receive an Optional<City> and must be updated to call .orElse(null), .orElseThrow(), or similar. This is a signature-breaking change across callers — the reviewer must update EntityController and any other call sites before merging.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/CityService.java around line 115, review and complete this code-review fix: CityService.getCityById() returns null instead of throwing or returning Optional.
What the draft fix changed: In `getCityById` (line 115), changed the return type from `City` to `Optional<City>` and rewrote the method body to return `cities.stream()...findFirst().map(this::populateState)`, eliminating all null returns. The `Optional` import was added. Risk: callers in `EntityController` (and any other callers not visible in this file) that currently do a null-check on the returned `City` will now receive an `Optional<City>` and must be updated to call `.orElse(null)`, `.orElseThrow()`, or similar. This is a signature-breaking change across callers — the reviewer must update `EntityController` and any other call sites before merging.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 82 medium — react 👍/👎 to teach the reviewer

Expand All @@ -159,12 +157,9 @@ public List<City> getCitiesByNearestTeamId(String teamId) {
return new ArrayList<>();
}
return cities.stream()
.filter(city -> {
String nearestTeamId = soccerTeamService.findNearestTeamId(city);
return teamId.equals(nearestTeamId);
})
.filter(city -> teamId.equals(city.getNearestTeamId()))
.map(this::populateState)
.sorted((a, b) -> Integer.compare(b.getPopulation(), a.getPopulation()))
.collect(Collectors.toList());
}
}
}