Problem
I need to get car info from a 3rd party web service and persist the data in my application DB. If my DB already has the car, I only update property values that may have changed. Otherwise, I create and save the car:
@Transactional //Spring Transaction
void saveCars() {
//get CarInfo from a webservice
List<CarInfo> carInfos = getCarInfo();
// now get each Car from my application DB
// that correspond to each CarInfo.
Set<Car> cars = mySpringCarRepo.findByCarInfoIdIn(
carInfos.stream().map(c -> c.getId())
.collect(Collectors.toSet());
for (CarInfo carInfo : carInfos) {
// If a car for the carInfo
// is already in my application
// just update it if some carInfo has changed.
// If a car is not there, create and save it to DB.
Car car = cars
.stream()
.filter(c -> c.getCarInfoId().equals(carInfo.getId())
.findAny()
.map(c -> updateCar(carInfo, car))
.orElseGet(() -> createCar(carInfo);
mySpringCarRepo.save(car);
}
}
I have the following questions / concerns about this code:
- Is this an inappropriate use of
Optional
‘smap
method? I’m not mapping to a new object but rather just returning the same car with some properties potentially updated. - The save method is really only required to be called for cars created in the
orElseGet
. Due to being in a transaction, the updates to the car will be persisted without the save call. It seems redundant and perhaps confusing to call save.
Effective Java 3rd edition states that many uses of isPresent
can profitably be replaced by other Optional
methods. So that has led me to the path of using map
& orElseGet
and the fact that Java 8 does not have a ifPresentOrElse
method on Optional
.
Solution
Regarding your questions
Is this an inappropriate use of Optional’s map method? I’m not mapping
to a new object but rather just returning the same car with some
properties potentially updated.
I think it’s appropriate because it might return an updated object. Unfortunately (as you said) Java 8 doesn’t have ifPresentOrElse
.
The save method is really only required to be called for cars created
in the orElseGet.
You can change the orElseGet
with orElseGet(() -> mySpringCarRepo.save(createCar(carInfo)))
and remove the final save to the db.
Performances
The complexity of the method is O(c*ci)
where c
is the number of cars in your DB and ci
is the number of CarInfo
.
If you store the Cars in a Map
instead of a Set
you can improve it to O(ci)
.
void saveCars() {
// get CarInfo from a webservice
List<CarInfo> carInfos = getCarInfo();
// Get cars from DB that correspond to each CarInfo
Map<Long,Car> cars = mySpringCarRepo
.findByCarInfoIdIn( getCarInfoIds(carInfos) )
.stream()
.collect( Collectors.toMap(Car::getCarInfoId,Function.identity()));
for (CarInfo carInfo : carInfos) {
// Returns Optional<Car>
Optional.ofNullable(cars.get(carInfo.getId()))
// Update car if already exists
.map(c -> updateCar(carInfo, c))
// Save car otherwise
.orElseGet(() -> mySpringCarRepo.save(createCar(carInfo)));
}
}
Or if you don’t mind to query the db for each CarInfo
, than the method is shorter:
void saveCars() {
getCarInfo().stream()
.forEach(carInfo ->
// returns Optional<Car>
mySpringCarRepo.findByCarInfoId(carInfo.getId())
// Update car if already exists
.map(car -> updateCar(carInfo, car))
// Save car otherwise
.orElseGet(() -> mySpringCarRepo.save(createCar(carInfo))));
}
Note 1: I haven’t fully tested the code, it’s just to give you an idea how to improve it.
Note 2: the performance gain might be irrelevant due to the DB and network latency, but with the second approach you should save some memory space.
Naming
The method name saveCars
is not very appropriate, a better name might be updateCars
or syncCars
.