Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This singleton is responsible for counting every pageview (called Routes in our case) we get by id, until the timer runs out and commits the results to the database in one query. Then the counter gets reset and the timer starts again.

I made this to reduce the amount of connections happening on our database at a given time.

Our pages are HttpServlet subclasses, so they get called concurrently. Our viewcounter should support this.

This is the code I think would be right for this:

@Singleton
@ConcurrencyManagement(BEAN)
public class RouteViewcounterSingleton {

    private ConcurrentHashMap<Integer, Integer> viewCounter;

    @EJB
    private RouteViewcountDAO routeViewcountDAO;

    @Inject
    private RouteDAO routeDao;

    @Resource
    TimerService timerService;

    @PostConstruct
    public void initialize() {
        viewCounter = new ConcurrentHashMap<>();
    }

    public void incrementViewCount(Integer routeId) {
        if (viewCounter.containsKey(routeId)) {
            Integer counter = viewCounter.get(routeId);
            counter++;
            viewCounter.put(routeId, counter);
        } else {
            viewCounter.put(routeId, 1);
        }
    }

    @Timeout
    public void programmaticTimeout(Timer timer) {
        Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, "ViewCounter timer timeout: " + DateTime.now(), "");
    }

    @Schedule(second = "0", minute = "*/10", hour = "*", info = "ViewCounterTimer")
    public void scheduleTimer() {
        if (viewCounter != null) {
            int i = 0;
            for (Map.Entry<Integer, Integer> entrySet : viewCounter.entrySet()) {
                try {
                    Integer routeId = entrySet.getKey();
                    Integer viewCount = entrySet.getValue();
                    Route route = routeDao.find(routeId);
                    RouteViewcount routeViewcount = routeViewcountDAO.find(route);
                    if (routeViewcount != null) {
                        routeViewcount.setViews(routeViewcount.getViews() + viewCount);
                        routeViewcountDAO.edit(routeViewcount);
                    } else {
                        routeViewcountDAO.create(new RouteViewcount(route, viewCount));
                    }
                    i++;
                } catch (MultipleResultsException | NoResultException | AttributeNotUniqueException | VersionConflictException ex) {
                    Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
            Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.INFO, "Viewcounter persisted for " + i + " routes on :" + DateTime.now(), "");
            viewCounter.clear();
        }
    }
}
share|improve this question

1 Answer 1

This method is broken in two ways. First, multiple threads can access the counter at the same time, and based on ordering, you may lose some updates. Second, if the counter is being created for the first time by multiple threads, you may create the counter multiple times.

public void incrementViewCount(Integer routeId) {
    if (viewCounter.containsKey(routeId)) {
        Integer counter = viewCounter.get(routeId);
        counter++;
        viewCounter.put(routeId, counter);
    } else {
        viewCounter.put(routeId, 1);
    }
}

In addition, you are not clear()ing the map in a thread-safe way. You should strongly consider an existing counter class, like Guava's AtomicLongMap. Writing a thread-safe counter is surprisingly non-trivial.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.