Local variables referenced from a lamba expression must be effectively final

[Al Hobbs]

Hi,

I am trying to filter a list based on a number in the object matching another number.  I have this code:

winners = list.stream()
                    .filter(u -> u.getHandStrength() == maxHand)
                    .collect(Collectors.toList());

It's telling me that the variable needs to be effectively final.  How should I approach this problem?

[Tim Cooke]

What variable?

[Campbell Ritchie]

Must be maxHand; you are obviously assigning it more than once.
When you use a variable in a λ, the λ, which can now be called a closure, captures the variable's value. If you reassign the variable the value inside the closure may be out of synch with the value outside; in the case of a local variable it may be impossible to resolve that confusion, so reassigning is not permissible. Until Java7, such local variables had to be declared final.

[Al Hobbs]

Sorry if my post was unclear.
Originally I thought it was because of the getHandStrength number being returned, but it must be maxHand.

final int maxHand = Collections.max(list.stream()
                    .filter(User::isPlaying)
                    .map(User::getHandStrength)
                    .collect(Collectors.toList()));

The stream I want to use is in a while loop, and the scope of maxHand is only in one loop, so I initialized it and declared in final when it was being made.
(I just added final int.)  It seems to have gotten rid of the problem.
Is that an appropriate solution?

Thanks for your replies!

[Campbell Ritchie]

Al Hobbs wrote:. . . The stream I want to use is in a while loop. . .

That isn't clear from your posted code, but it doesn't strike me as correct to use a mixture of Stream and loops.

the scope of maxHand is only in one loop, so I initialized it and declared in final when it was being made.
(I just added final int.)  It seems to have gotten rid of the problem.
Is that an appropriate solution?

Thanks for your replies!

Marking a variable final is usually a good solution, so keep the final modifier, but I think there is a better way to do things. If getHandStrength() returns an int, I suggest you use a different method from map. Try this, which returns a different kind of Stream, which has a method to calculate the maximum if present. You may end up writing something like orElse(0) in case nobody is playing at all.
The result from getHandStrength() is not reassigned in the Stream, so that won't be the problem.

[Al Hobbs]

final int maxHand = list.stream()
                    .filter(User::isPlaying)
                    .mapToInt(User::getHandStrength)
                    .max().getAsInt();

I changed how I got maxHand.  It looks a lot simpler than using Collections.max() on the map.
Sorry, the loop performs other operations in which an updated list is needed, so it's not directly part of the filtering process.
I don't plan on using orElse() because ideally it wouldn't get to that point if there is nobody playing.
Thanks!!

[Stephan van Hulst]

You can also do this in a single reduction, if you're willing to use more memory during the reduction:

List<User> winners = players.stream()
  .collect(
    groupingBy(User::getHandStrength, TreeMap::new, toList()))
  .lastEntry()
  .getValue();

Or if you're willing to write your own collector:

  static <T, A, R> Collector<T, ?, R> multiMaxBy(Comparator<? super T> comparator, Collector<? super T, A, R> downstream) {
    final class Accumulation {

      private A accumulation;
      private T max;
      
      private boolean isEmpty() {
        return accumulation == null;
      }

      void accumulate(T next) {
        int comparison = isEmpty() ? -1 : comparator.compare(max, next);

        if (comparison > 0)
          return;

        if (comparison < 0) {
          accumulation = downstream.supplier().get();
          max = next;
        }

        downstream.accumulator().accept(accumulation, next);
      }

      Accumulation combine(Accumulation other) {
        if (this.isEmpty())
          return other;

        if (other.isEmpty())
          return this;

        int comparison = comparator.compare(this.max, other.max);

        if (comparison > 0)
          return this;

        if (comparison < 0)
          return other;
        
        this.accumulation = downstream.combiner().apply(this.accumulation, other.accumulation);
        return this;
      }

      R finish() {
        return downstream.finisher().apply(this.accumulation);
      }
    }

    return Collector.of(Accumulation::new, Accumulation::accumulate, Accumulation::combine, Accumulation::finish);
  }

List<User> winners = players.stream().collect(
  multiMaxBy(
    comparingInt(User::getHandStrength),
    toList()
  ));

The latter is obviously much more verbose, but the upside is that the multiMaxBy() method is reusable. You can call it wherever you need to get more than one maximal element from a stream.

[Stephan van Hulst]

I'm actually really peeved that the Collectors class doesn't already have the version I wrote above, and just implements maxBy() like this:

public static <T> Collector<T, ?, Optional<T>> maxBy(Comparator<? super T> comparator) {
  return multiMaxBy(
    comparator,
    reducing(
      Optional.empty(),
      Optional::of,
      (first, second) -> first.isPresent() ? first : second
    ));
}

[Campbell Ritchie]

Al Hobbs wrote:

final int maxHand = list.stream()
                    .filter(User::isPlaying)
                    .mapToInt(User::getHandStrength)
                    .max().getAsInt();

. . .

A lot better. I still think I'd have used orElse(), otherwise you are risking an exception if nobody is playing.

[Al Hobbs]

Stephan van Hulst wrote:You can also do this in a single reduction, if you're willing to use more memory during the reduction:

So you're saying that those two functions would give a list of winners with the highest hand?  I'll have to look over how you created the collector because I have never seen that before.

[Campbell Ritchie]

You can collect the object passed by the Stream into a Map and use a second Collector to go for the maximum.

At which point this discussion becomes scary for the beginning forum, so let's move you to our Java8 forum.

[Stephan van Hulst]

Al Hobbs wrote:So you're saying that those two functions would give a list of winners with the highest hand?  I'll have to look over how you created the collector because I have never seen that before.

Yes, both ways use a single reduction, instead of calculating the maximal hand value first.

If you need help understanding the collector, feel free to ask questions.