Threadsafe get method on queue that draws values from other queues?

I have a class that implements Queue and draws values from other queues which may still be referenced outwith it. I want my method to draw values from the contained queues, using synchronized locks on them to ensure thread safety with other code that uses synchronized locks on the queues.

The way I've tried to achieve this is by having my method loop through all values indefinitely, storing the value if it's the next one, updating the stored values if it reaches the same queue again and the variables the queues are being measured by have changed since the list iteration, and returning the next value of the queue if it's unchanged since the last time it was checked and evaluated to have the next value - my logic being that at that point, all queues have been checked and the currently stored value/queue was checked earliest and has been reconfirmed to be the next value.

Is this the ideal way to create a thread-safe version of this method? Or would there be a better way?

http://pastebin.com/TSkiJFh0

```
Collection> memberQueues;
final Lambda keyGetter;

public T get(boolean remove)
{
// workaround to ensure thread-safety when synchronized locks can't extend past the block they're declared in.

// Work on a copy of memberQueues
List> memberQueuesCopy;

synchronized(memberQueues)
{ memberQueuesCopy = new ArrayList>(memberQueues); }

// Declare variables and initialise with last member of memberQueuesCopy.
// If it checks every variable and the last one is next, then I don't think I need to check again.
Queue nextQueue = memberQueuesCopy.get(memberQueuesCopy.size() - 1);
T nextValue = nextQueue.peek();
Comparable nextValueComparable = keyGetter.getMember(nextValue);

// Check all members of memberQueuesCopy. Find the lowest and hold the value until it gets to it again incase
// any values have changed. When it gets back to the current lowest value, check whether the value it's being
// sorted by has changed - and if

Code Style

Java Code Style puts the open-brace at the end of the line, not the start of the next line. For example, you have:

if(i == nextQueue)
            {

but that should be:

if(i == nextQueue) {

Variable conventions

i as a variable name is a great idea, if the variable is the control integer in a for loop. In your case, I presume it is short for 'item', or something, but, a Queue, being called i is unconventional.

As it happens, the letter q is perfect as a substitute....

Now, your nextQueue variable is actually the lastQueue odd.

Function extraction

With synchronization, return-balues from methods are often a great help for readibility. Consider this code you have:

// Work on a copy of memberQueues
List> memberQueuesCopy;

synchronized(memberQueues)
{ memberQueuesCopy = new ArrayList>(memberQueues); }

Which should really be written as:

// Work on a copy of memberQueues
List> memberQueuesCopy;

synchronized(memberQueues) {
    memberQueuesCopy = new ArrayList>(memberQueues);
}

would be even better if written as:

private final List> copyQueues() {
    synchronized(memberQueues) {
        return new ArrayList>(memberQueues);
    }
}

and then:

// Work on a copy of memberQueues
List> memberQueuesCopy = copyQueues();

Bugs

There are three bugs I should point out:

-
NoSuchElementException if memberQeues is empty:


Queue nextQueue = memberQueuesCopy.get(memberQueuesCopy.size() - 1);

-
(and bug 3) NullPointerException if any of the queues are empty (in some combinations) (one bug on iComparable, the other on nextValueComparable):

T nextValue = nextQueue.peek();
Comparable nextValueComparable = keyGetter.getMember(nextValue);

  ....

    T iValue = i.peek();
    Comparable iComparable = keyGetter.getMember(iValue);

  ....

    if(nextValue == iValue && nextValueComparable.equals(iComparable))

  ....

    if(iComparable.compareTo(nextValueComparable) < 0)

StackExchange Code Review Q#56054, answer score: 5