HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaspringMinor

Bot-like Spring Java service for checking advertisements

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
springadvertisementscheckinglikejavabotservicefor

Problem

I am in the middle of developing a Spring Boot application, which has a service acting like a bot.

advertisementService.update(Advertisement ad) is a method which parses some HTML. It is important that it cannot execute periodically. For now it is pseudo random, but in the future there will be some logic returning the time depending on information read in previous parsing.

Immediately after the start of the app, it invokes the method scheduleAllChecks().
If a new Advertisement comes up, then it calls scheduleCheck(0, ad);

I am wondering if the solution (I mean the whole service class) is proper and if it can be beter?

```
package com.gadomski.service;

import com.gadomski.model.Advertisement;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

import java.util.Random;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

/**
* Created by Krzysiek on 21.08.2016.
*/

@Service
public class CheckerService {
private final Logger log = LoggerFactory.getLogger(this.getClass());
private final int CHECKER_THREADS = 100;
@Autowired
AdvertisementService advertisementService;
private ScheduledExecutorService scheduledExecutorService;

CheckerService() {
this.scheduledExecutorService = Executors.newScheduledThreadPool(CHECKER_THREADS);
}

public void scheduleCheck(int delaySeconds, Advertisement advertisement) {
if (advertisement.isActive()) {
ScheduledFuture check = this.scheduledExecutorService.schedule(
() -> {
scheduleCheck(getDelayInSeconds(), advertisement);
advertisementService.updateAdvertisement(advertisement);
log.info("Scheaduled next check for " + advertisement.getUrl()

Solution

It appears that you are using Random across multiple threads. I guess you plan to replace it in the future, but still, according to the documentation:


Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs.

You should probably rewrite this to use java.util.concurrent.ThreadLocalRandom. As the docs say:


When applicable, use of ThreadLocalRandom rather than shared Random objects in concurrent programs will typically encounter much less overhead and contention.

Context

StackExchange Code Review Q#147240, answer score: 2

Revisions (0)

No revisions yet.