1
\$\begingroup\$

I just want to train before interview. I googled concurrency interview questions and one of the question was write concurrent ready stack.

public interface ConcurrentStack<T> {
    void put(T t) throws InterruptedException;

    T poll() throws InterruptedException;
}

class BlockingConcurrentStack<T> implements ConcurrentStack<T> {

    private List<T> list = new ArrayList<>();
    private int maxSize;

    public BlockingConcurrentStack(int maxSize) {
        this.maxSize = maxSize;
    }

    @Override
    public synchronized void put(T t) throws InterruptedException {
        while (maxSize <= list.size()) {                
            wait();
        }
        list.add(t);        
        notify();

    }

    @Override
    public synchronized T poll() throws InterruptedException {
        while (list.size() == 0) {
            wait();
        }
        T t = list.get(list.size() - 1);
        list.remove(t);           
        notify();
        return t;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Just short comments then I post them here instead of an answer: \$\endgroup\$ – Adriano Repetti Feb 6 '17 at 15:39
  • \$\begingroup\$ I'd expect poll() to have timeout and to return null if there is nothing to pull out from the collection but your implementation has not. To be consistent with other concurrent collections I'd call it take() instead. \$\endgroup\$ – Adriano Repetti Feb 6 '17 at 15:39
  • \$\begingroup\$ Also you offer only blocking methods (put() and poll() which should be renamed take()) while often a non-blocking throwing/ method is useful, I'd then add both add() and poll(). I'd even go a step further and I'd implement all the other useful methods (like offer() and peek()) but it's probably just over-engineering at this stage. \$\endgroup\$ – Adriano Repetti Feb 6 '17 at 15:39
  • \$\begingroup\$ I'd remove logging code from there. You're tied to console (which might be useful during developing but for sure it's not right on production code). Use it while writing your class but don't present it in your answer...You may also want (or at least consider) to implement ConcurrentStack<T> and Collection<T>. \$\endgroup\$ – Adriano Repetti Feb 6 '17 at 15:40
1
\$\begingroup\$

There is only one concern for me: you should not allow to instantiate the object with max elements below 0 (IllegalArgumentException ). Therefore a proper handling for this situation in your methods has to be made to avoid a neverending blocking. I suggest to return null.

Expecting a timout for your poll-method may be beneficial in several situations. Concurrent implementations of the Java API do define both: a non-argument poll-method and one with a timeout. But I do not see this as mandatory.

Some developers will suggest to make synchronization intransparent to the user of your class introducing an internal lock object and mention thread-safety in JavaDoc. They argue with "defensive programming" to prevent deadlocks. My personal opinion to that: You send the wrong message to the using developer.

Don't get me wrong: Preventing deadlocks is important. But it is more important to know what to do when. That is known as competence. Preventing others to make mistakes is ok. That can be done in several ways. But others to allow a lack of competence is exactly the message you would send. You would try to solve a potential future errors that are not your business. On the other side you allow misusage of objects without habing the other force to correct thhis. My way is: If someone is doing strange things with objects he doesn't own he either should know exactly what he is doing or handle occuring errors and deadlocks. You should know that this is how the majority of developers will NOT argue.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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