Skip to content

fix(MAJORLEA-013): 6 review findings across 3 files #74

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 3 commits into
base: main
Choose a base branch
from
+46 −31
Draft

fix(MAJORLEA-013): 6 review findings across 3 files #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75111e0
fix(MAJORLEA-013): 6 review findings across 3 files
flamingo[bot] Aug 14, 2026
7bba67e
fix(MAJORLEA-013): 6 review findings across 3 files
flamingo[bot] Aug 14, 2026
704c57d
fix(MAJORLEA-013): 6 review findings across 3 files
flamingo[bot] Aug 14, 2026
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
8 changes: 4 additions & 4 deletions backend/src/main/java/cx/flamingo/analysis/service/LanguageService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -72,11 +73,10 @@ public List<Language> autocompleteLanguages(String query, int maxResults) {
.collect(Collectors.toList());
}

public Language getLanguageById(String id) {
public Optional<Language> getLanguageById(String id) {
return languages.stream()
.filter(l -> l.getId().equals(id))
.findFirst()
.orElse(null);
.findFirst();
}

public List<Language> getAllLanguages() {
Comment on lines 73 to 82

@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.

🦩 🔴 LanguageService.getLanguageById() returns null instead of Optional or throwing

Changed getLanguageById(String id) return type from Language to Optional<Language> and replaced .orElse(null) with .findFirst() directly (line 62-65). Added import java.util.Optional; at line 10. The method itself is now null-safe. RISK: Any callers of getLanguageById() in other files (controllers, services, etc.) that previously used the returned Language directly will now receive an Optional<Language> and will fail to compile until updated to call .get(), .orElseThrow(), or similar. Those callers are not visible in this file and must be updated separately. A reviewer should search the codebase for all usages of getLanguageById before merging.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/LanguageService.java around line 62, review and complete this code-review fix: LanguageService.getLanguageById() returns null instead of Optional or throwing.
What the draft fix changed: Changed `getLanguageById(String id)` return type from `Language` to `Optional<Language>` and replaced `.orElse(null)` with `.findFirst()` directly (line 62-65). Added `import java.util.Optional;` at line 10. The method itself is now null-safe. RISK: Any callers of `getLanguageById()` in other files (controllers, services, etc.) that previously used the returned `Language` directly will now receive an `Optional<Language>` and will fail to compile until updated to call `.get()`, `.orElseThrow()`, or similar. Those callers are not visible in this file and must be updated separately. A reviewer should search the codebase for all usages of `getLanguageById` before merging.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -89,4 +89,4 @@ public Language getDefaultLanguage() {
.findFirst()
.orElseThrow(() -> new RuntimeException("Default language (Java) not found"));
}
}
}
40 changes: 24 additions & 16 deletions backend/src/main/java/cx/flamingo/analysis/service/RegionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,35 +5,39 @@
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.core.io.ClassPathResource;
import org.springframework.stereotype.Service;

import cx.flamingo.analysis.model.City;
import cx.flamingo.analysis.model.Region;
import jakarta.annotation.PostConstruct;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;

@Slf4j
@Service
@RequiredArgsConstructor
public class RegionService {
private List<Region> regions;
private Map<String, Integer> regionPopulationCache;

@Autowired
private StateService stateService;

@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.

🦩 🟠 RegionService uses @Autowired field injection instead of @requiredargsconstructor

Removed both @Autowired annotations from stateService and cityService fields, changed them to private final, added @RequiredArgsConstructor to the class annotation, and removed the import org.springframework.beans.factory.annotation.Autowired; import. Added import lombok.RequiredArgsConstructor;. This is a mechanical, low-risk change consistent with Lombok constructor injection conventions.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/RegionService.java around line 28, review and complete this code-review fix: RegionService uses @Autowired field injection instead of @RequiredArgsConstructor.
What the draft fix changed: Removed both `@Autowired` annotations from `stateService` and `cityService` fields, changed them to `private final`, added `@RequiredArgsConstructor` to the class annotation, and removed the `import org.springframework.beans.factory.annotation.Autowired;` import. Added `import lombok.RequiredArgsConstructor;`. This is a mechanical, low-risk change consistent with Lombok constructor injection conventions.
Verify the change is correct and complete; do not refactor unrelated code.

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

private final StateService stateService;

@Autowired
private CityService cityService;
private final CityService cityService;

@PostConstruct
public void init() {
loadRegions();
log.info("Loaded {} regions", regions.size());
buildPopulationCache();
}

private void loadRegions() {
Expand Down Expand Up @@ -71,6 +75,15 @@ private void loadRegions() {
}
}

private void buildPopulationCache() {
regionPopulationCache = new HashMap<>();
for (City city : cityService.getAllCities()) {
for (String regionId : city.getRegionIds()) {
regionPopulationCache.merge(regionId, city.getPopulation(), Integer::sum);
}
}
}

public void updateRegion(Region updatedRegion) {
int index = -1;
for (int i = 0; i < regions.size(); i++) {
Comment on lines 75 to 89

@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.

🦩 🟠 RegionService.getRegionTotalPopulation() performs a full scan of all cities for every region during autocomplete sorting — O(n*m) complexity

Added a private Map<String, Integer> regionPopulationCache field and a buildPopulationCache() method that pre-computes regionId → total population by iterating all cities once (O(C)) and accumulating via Map.merge. buildPopulationCache() is called at the end of @PostConstruct init(). getRegionTotalPopulation(Region) now does a single O(1) map lookup via regionPopulationCache.getOrDefault(region.getId(), 0) instead of a full city scan. Added import java.util.HashMap; and import java.util.Map;. Risk: the cache is built once at startup and is not invalidated if cities are updated at runtime; if cityService data is mutable after init, the cache could become stale. Based on the visible code this appears to be static CSV-loaded data, so this is acceptable.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/RegionService.java around line 80, review and complete this code-review fix: RegionService.getRegionTotalPopulation() performs a full scan of all cities for every region during autocomplete sorting — O(n*m) complexity.
What the draft fix changed: Added a `private Map<String, Integer> regionPopulationCache` field and a `buildPopulationCache()` method that pre-computes regionId → total population by iterating all cities once (O(C)) and accumulating via `Map.merge`. `buildPopulationCache()` is called at the end of `@PostConstruct init()`. `getRegionTotalPopulation(Region)` now does a single O(1) map lookup via `regionPopulationCache.getOrDefault(region.getId(), 0)` instead of a full city scan. Added `import java.util.HashMap;` and `import java.util.Map;`. Risk: the cache is built once at startup and is not invalidated if cities are updated at runtime; if `cityService` data is mutable after init, the cache could become stale. Based on the visible code this appears to be static CSV-loaded data, so this is acceptable.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -88,10 +101,7 @@ public void updateRegion(Region updatedRegion) {
}

private int getRegionTotalPopulation(Region region) {
return cityService.getAllCities().stream()
.filter(city -> city.getRegionIds().contains(region.getId()))
.mapToInt(City::getPopulation)
.sum();
return regionPopulationCache.getOrDefault(region.getId(), 0);
}

public List<Region> autocompleteRegions(String query, String stateId, List<String> cityIds, int maxResults) {
Expand Down Expand Up @@ -122,21 +132,19 @@ public List<Region> autocompleteRegions(String query, String stateId, List<Strin
.collect(Collectors.toList());
}

public Region getRegionById(String id) {
public Optional<Region> getRegionById(String id) {
return regions.stream()
.filter(r -> r.getId().equals(id))
.findFirst()
.orElse(null);
.findFirst();
}

public Region getRegionByName(String name) {
public Optional<Region> getRegionByName(String name) {
return regions.stream()
.filter(r -> r.getName().equalsIgnoreCase(name))
.findFirst()
.orElse(null);
.findFirst();
}

public List<Region> getAllRegions() {
return new ArrayList<>(regions);
}
}
}
Comment on lines 132 to +150

@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.

🦩 🔴 RegionService.getRegionById() and getRegionByName() return null instead of Optional or throwing

Changed getRegionById(String id) and getRegionByName(String name) to return Optional<Region> instead of Region, replacing .orElse(null) with .findFirst() directly (which already returns Optional<Region>). Added import java.util.Optional;. The return-type change is correct and complete within this file, but callers of these methods in other files (not visible here) will now receive Optional<Region> and must be updated to call .get(), .orElseThrow(), or similar — those callers will fail to compile until updated. A reviewer must check all call sites before merging.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/RegionService.java around line 113, review and complete this code-review fix: RegionService.getRegionById() and getRegionByName() return null instead of Optional or throwing.
What the draft fix changed: Changed `getRegionById(String id)` and `getRegionByName(String name)` to return `Optional<Region>` instead of `Region`, replacing `.orElse(null)` with `.findFirst()` directly (which already returns `Optional<Region>`). Added `import java.util.Optional;`. The return-type change is correct and complete within this file, but callers of these methods in other files (not visible here) will now receive `Optional<Region>` and must be updated to call `.get()`, `.orElseThrow()`, or similar — those callers will fail to compile until updated. A reviewer must check all call sites before merging.
Verify the change is correct and complete; do not refactor unrelated code.

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

29 changes: 18 additions & 11 deletions backend/src/main/java/cx/flamingo/analysis/service/StateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -24,6 +27,7 @@
@Service
public class StateService {
private List<State> states;
private Map<String, Integer> statePopulationCache;

private final CityService cityService;

Expand All @@ -36,6 +40,7 @@ public StateService(@Lazy CityService cityService) {
public void init() {
loadStates();
log.info("Loaded {} states", states.size());
buildPopulationCache();
}

private void loadStates() {
Expand Down Expand Up @@ -70,11 +75,15 @@ private void loadStates() {
}
}

@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.

🦩 🟠 StateService.getStateTotalPopulation() performs a full scan of all cities for every state during autocomplete sorting — O(n*m) complexity

Added buildPopulationCache() called from @PostConstruct init() after loadStates(). Added field private Map<String, Integer> statePopulationCache and method buildPopulationCache() which iterates all cities once and accumulates population per stateId using Map.merge. Changed getStateTotalPopulation(State) to do an O(1) statePopulationCache.getOrDefault(state.getId(), 0) lookup instead of a full city scan. Added import java.util.HashMap; and import java.util.Map;. The cache is built once at startup; if city data changes at runtime the cache would be stale, but given the existing @PostConstruct-only loading pattern this is consistent with the rest of the service.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/StateService.java around line 72, review and complete this code-review fix: StateService.getStateTotalPopulation() performs a full scan of all cities for every state during autocomplete sorting — O(n*m) complexity.
What the draft fix changed: Added `buildPopulationCache()` called from `@PostConstruct init()` after `loadStates()`. Added field `private Map<String, Integer> statePopulationCache` and method `buildPopulationCache()` which iterates all cities once and accumulates population per `stateId` using `Map.merge`. Changed `getStateTotalPopulation(State)` to do an O(1) `statePopulationCache.getOrDefault(state.getId(), 0)` lookup instead of a full city scan. Added `import java.util.HashMap;` and `import java.util.Map;`. The cache is built once at startup; if city data changes at runtime the cache would be stale, but given the existing `@PostConstruct`-only loading pattern this is consistent with the rest of the service.
Verify the change is correct and complete; do not refactor unrelated code.

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

private void buildPopulationCache() {
statePopulationCache = new HashMap<>();
for (City city : cityService.getAllCities()) {
statePopulationCache.merge(city.getStateId(), city.getPopulation(), Integer::sum);
}
}

private int getStateTotalPopulation(State state) {
return cityService.getAllCities().stream()
.filter(city -> city.getStateId().equals(state.getId()))
.mapToInt(City::getPopulation)
.sum();
return statePopulationCache.getOrDefault(state.getId(), 0);
}

public List<State> autocompleteStates(String query, String regionId, List<String> cityIds, int maxResults) {
Expand Down Expand Up @@ -105,21 +114,19 @@ public List<State> autocompleteStates(String query, String regionId, List<String
.collect(Collectors.toList());
}

@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.

🦩 🔴 StateService.getStateById() and getStateByCode() return null instead of Optional or throwing

Changed getStateById() and getStateByCode() to return Optional<State> instead of State, replacing .orElse(null) with .findFirst() directly (which already returns Optional<State>). Added import java.util.Optional;. The return-type change is correct and complete within this file, but callers in other files (not visible here) that previously received a State directly will now receive Optional<State> and will fail to compile until updated. The reviewer must locate all callers (e.g. via getStateById(...) and getStateByCode(...) usages across the codebase) and update them to use .orElseThrow(...) or .orElse(...) as appropriate. This is a breaking API change that cannot be fully resolved in this file alone.

🤖 Prompt for AI agents 
In backend/src/main/java/cx/flamingo/analysis/service/StateService.java around line 107, review and complete this code-review fix: StateService.getStateById() and getStateByCode() return null instead of Optional or throwing.
What the draft fix changed: Changed `getStateById()` and `getStateByCode()` to return `Optional<State>` instead of `State`, replacing `.orElse(null)` with `.findFirst()` directly (which already returns `Optional<State>`). Added `import java.util.Optional;`. The return-type change is correct and complete within this file, but callers in other files (not visible here) that previously received a `State` directly will now receive `Optional<State>` and will fail to compile until updated. The reviewer must locate all callers (e.g. via `getStateById(...)` and `getStateByCode(...)` usages across the codebase) and update them to use `.orElseThrow(...)` or `.orElse(...)` as appropriate. This is a breaking API change that cannot be fully resolved in this file alone.
Verify the change is correct and complete; do not refactor unrelated code.

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

public State getStateById(String id) {
public Optional<State> getStateById(String id) {
return states.stream()
.filter(s -> s.getId().equals(id))
.findFirst()
.orElse(null);
.findFirst();
}

public State getStateByCode(String code) {
public Optional<State> getStateByCode(String code) {
return states.stream()
.filter(s -> s.getCode().equalsIgnoreCase(code))
.findFirst()
.orElse(null);
.findFirst();
}

public List<State> getAllStates() {
return new ArrayList<>(states);
}
}
}